Why is plateXOffset hardcoded?

[zepumph]

from code review #30
I'm trying to figure out the best practices for layout. I thought that whenever possible it is good to use percentages vs hard coded numbers. In both atoms screen views I see plateXOffset for the distance between the molecule and the e-field plates on either side. Would it be better for this to be based on the screen layoutBounds' width? Can you speak to best practices here? I'm sure it's very sim conditional.

[pixelzoom]

You can't avoid all hard-coded numbers. Things like spacing, margin, font size, etc. are often appropriate to choose "empirically" (by visual inspection), based on aesthetics. In my more recent code, I typically document these types of constants with something like "determined empirically". And one should avoid duplicating such hard-coded values if they appear in more than one place; define var constants. That said, let's look at this specific example.

TwoAtomsScreenView is current doing something like this to position the E-field plates:

var plateXOffset = 250;
negativePlateNode.right = model.molecule.location.x - plateXOffset;
positivePlateNode.right = model.molecule.location.x + plateXOffset;

That approach assumes that our molecule is never going to change size. If (for example) we were to increase the diameter of atoms (or length of bonds) in the future, the layout is likely to be broken - the plates may overlap the molecule. Not a big deal because we'd see the problem immediately. But someone would need to find and tweak plateXOffset to fix the overlap.

A better general approach might be to locate the plates some distance from the outer radius of the molecule. Something like this:

var PLATE_X_SPACING = 75; // space between molecule and E-field plates, determined empirically
var moleculeRadius = model.molecule.getRadius(); //NOTE: getRadius doesn't exist
negativePlateNode.right = model.molecule.location.x - moleculeRadius - PLATE_X_SPACING;
negativePlateNode.left = model.molecule.location.x + moleculeRadius + PLATE_X_SPACING;

In this case, molecule.getRadius doesn't exist, and it would be complicated to implement. And even if the plates were positioned based on the molecule radius, there would be other potential problems caused by a larger molecule: the left plate and right control panel might be pushed outside the ScreenView's layoutBounds. (Layout is sort of like "whack a mole".) This is an old sim ported from Java, so the probability that the molecule sizes are going to increase is close to nil, so I think the layout approach is acceptable "as is".

A couple of improvements that I've made... Rather than being defined in the constructors, plateXOffset should be a constant named PLATE_X_OFFSET. I've made that change, and documented the constants with this issue number.

@zepumph please review, let me know if you have any other related questions.

[zepumph]

Thank you! Very comprehensive and just what I was looking for.

To sum up, much of the layout strategies you employ are based on the maintainability of the code.

In general, would having hard coded numbers like this cause problems if this sim was used in different sized iframes (perhaps with different aspect ratios)? Do we even try to support that?

[pixelzoom]

We do all layout based on ScreenView.layoutBounds, which corresponds to iPad screen dimensions.

The default is defined in ScreenView.js:

/*
   * Default width and height for iPad2, iPad3, iPad4 running Safari with default tabs and decorations
   * These bounds were added in Sep 2014 and are based on a screenshot from a non-Retina iPad, in Safari, iOS7.
   * It therefore accounts for the nav bar on the bottom and the space consumed by the browser on the top.
   * As of this writing, this is the resolution being used by PhET's sim designers for their mockups.
   * For more information see https://github.com/phetsims/joist/issues/126
   */
  var DEFAULT_LAYOUT_BOUNDS = new Bounds2( 0, 0, 1024, 618 );

[pixelzoom]

Some sims do attempt to maximize use of space outside ScreenView.layoutBounds. For example, projectile-motion floats its control panel as far right as possible, to maximize the space available for shooting projectiles.

[zepumph]

In MPConstants I see that this default is being overridden, is this what you want for MP?

SCREEN_VIEW_OPTIONS: { layoutBounds: new Bounds2( 0, 0, 1100, 700 ) },

It looks pretty minor, but it has been around since 2/3/14, probably before the default was set in stone?

[pixelzoom]

It's common to override the layoutBounds when porting a Java sim. Java sims had a similar concept, and using the same dimensions as the Java sim means that all of the layout, font sizes, etc. will look the same when ported to Javascript.

[zepumph]

Sounds good, closing