Panel: change default for option.align to 'left'

[pixelzoom]

Panel has option align, which currently defaults to 'right':

32 align: 'right', // {string} horizontal alignment of content in the pane, 'left'|'center'|'right'

This seems like the least likely of the 3 values, and likely to cause i18n layout bugs (e.g. phetsims/plinko-probability#52). Why is 'right' the default? And should we change it?

[pixelzoom]

I spot checked a dozen or so panels, and they are all setting align to 'left' or 'center'. But there are 150 occurrences of "var Panel = require", so determining whether changing the default to (e.g.) 'center' would affect any sims would be time consuming. Does anyone recall using a Panel that needs to be right aligned?

[pixelzoom]

Looking at GitHub history... The align option appeared for awhile with default 'center', then disappeared for awhile, then was resurrected (inadvertently?) with with default 'right'. There are a bunch of commits with message "Merge remote-tracking branch 'origin/master'", so it's possible that there was some funny business going on here.

[samreid]

The align option appeared for awhile with default 'center', then disappeared for awhile, then was resurrected (inadvertently?) with with default 'right'

How are you seeing this? When I show "history for selection", it indicates that a5e0a77 introduced align:right and it has remained so ever sense. Here's the issue comment that describes that commit: #158 (comment)

[samreid]

Does anyone recall using a Panel that needs to be right aligned?

Perhaps it was to get checkboxes to line up?

[pixelzoom]

@samreid I don't follow... How does align:'right' cause check boxes to line up? That would be align:'left'.

[pixelzoom]

@samreid asked:

How are you seeing this?

You didn't look back far enough in the history. The commit you referenced (@a5e0a77c29c9e58dd8082f7e6cc595103db36b79) is from 2/24/15. The align option was added in @14d7e60e4940953881e2f870ea299f79438edd91 (11/27/13) with default value 'center', then was "backed out" in @d75cc36443a8be0225699ea0a79ef1bccb63264c (11/29/13).

[samreid]

My mistake, I thought the checkboxes were on the other side.

I see the history now, thanks!

[pixelzoom]

8/25/16 dev meeting:
Change to 'left', add assertion in Panel to check for any cases when options.align is not specified and relying on 'right' default, run test-server. I'll do this.

[samreid]

You can get a stack trace at any point by doing this trick (from phetAllocation.js)

      var stack;
      try { throw new Error(); }
      catch( e ) { stack = e.stack; }

[pixelzoom]

I (temporarily) added this to the first line of Panel's constructor, to catch cases where sims are relying on the default of align:'right'. I'll run test-server, and create sim-specific issues.

    if ( !options || !options.align ) {
      try {
        throw new Error();
      }
      catch( e ) {
        console.log( '\n\n\n>>>> sim relies on align:right' );
        console.log( e.stack );
      }
    }

[pixelzoom]

As you can see from the issues I created above, many sims are using the old default of align:'right'. Some (most?) of these may be unaffected by the change to align:'left'. But I don't have time to check them all. So I've assigned issues to the developers responsible for each sim.

[samreid]

How does align:'right' cause check boxes to line up? That would be align:'left'.

I think we've both misinterpreted the align values for Panel. The Panel constructor is:

function Panel( content, options ) {

The panel takes a single child, the align is choosing where to put that single child within the bounding rectangle of the Panel.

I modified the layout code to show the difference between left/center/right like so:

      // center
      content.center = background.center;
      console.log(content.left);

      // left
      content.left = background.left + options.xMargin;
      content.centerY = background.centerY;
      console.log(content.left);

      // right
      content.right = background.right - options.xMargin;
      content.centerY = background.centerY;
      console.log(content.left);

When running with Balloons and Static Electricity, I saw these values:

5
4.5
5.5

Based on this new information, it seems we must re-discuss what to do, since the align actually has nothing to do with how elements in the content align. Perhaps we should default to center, meaning that the panel content is centered in the Panel. We may wish to leave left/right as options in case a developer needs to make an asymmetrical Panel with empty space on one side or the other? If not, then perhaps we can remove the align option altogether.

Bumping to high-priority to discuss with @pixelzoom before @jbphet @jessegreenberg @andrewadare @aadish @jonathanolson or other developers spend a lot of time investigating their align values.

[pixelzoom]

Correct, align does NOT operate on the content. And it's largely irrelevant -- except when you specify the minWidth option. If minWidth > content.width, then you'll see the effects of align.

So I'm going to rerun with this check as the first line of Panel's constructor:

    // If you specified a minWidth and didn't specify align, 
    // then behavior will change when the default align value is changed.
    if ( options && options.minWidth && !options.align ) {
      try {
        throw new Error();
      }
      catch( e ) {
        console.log( '\n\n\n>>>> sim relies on align:right' );
        console.log( e.stack );
      }
    }

[pixelzoom]

I verified that the above test is correct by commenting out align:'left' at this call site in ABSControlPanel, which also specifies minWidth:

    var panelOptions = {
      minWidth: Math.max( solutionControl.width, Math.max( viewsControl.width, toolsControl.width ) ) + ( 2 * xMargin ),
      fill: ABSColors.CONTROL_PANEL_BACKGROUND,
      xMargin: xMargin,
      yMargin: 6,
      // align: 'left'
    };

Then I verified that the layout of the control panels was affected. See screenshot below, where the Views panel is now (undesirably) right aligned.

[pixelzoom]

With the new error test, the sims that may have problems are:

• atomic-interactions (@jbphet)
• fluid-pressure-and-flow (@samreid)
• molecules-shapes (@jonathanolson)
• states-of-matter (@jbphet)
• under-pressure (@samreid)

To be clear: This sims may have a problem because they were using Panel's minWidth option while not specifying the align options. Therefore the alignment will have changed if the panels content width exceeds minWidth.

[pixelzoom]

I visually inspected the 5 above sims, and I see no adverse affects of changing the default to align:'left'. I've left the associated issues open (and assigned) in case the primary developers want to confirm for themselves.

Work is done here. @samreid would you mind sanity checking? The only direct commit is 39970e6.

[samreid]

I think we should change the default alignment to "center". This refers to whether the content is centered inside the panel, and should nearly always be true, unless a simulation has a need for asymmetry in the panel.

[pixelzoom]

Disagree. align is relevant when minWidth is used. minWidth is typically used to create multiple panels that have the same width. And in that case, PhET typically wants components in the panels to left align with each other. Examples:

acid-base-solutions:

under-pressure:

balancing-act:

[samreid]

minWidth is typically used to create multiple panels that have the same width

This sounds like it would lead to control panels that have differing font sizes, which sounds like poor design. Can you show some examples of this so I can take a look? My memory is fuzzy and I can't find examples at the moment, but I thought I've used a pattern that adds a horizontal strut to make sure the panels match widths without having different font sizes.

And in that case, PhET typically wants components in the panels to left align with each other.

I agree completely with this, but this should be a natural consequence of using align: center with unscaled panels (and contents?)

[samreid]

If using minWidth to match panel widths is a common pattern and the different font sizes is not a problem, and it doesn't matter if the content is exactly centered in the panel, then align: left sounds reasonable. But I'm also wondering: why do align: left and align: center give different results when minWidth is not used?

[pixelzoom]

Example in ABSControlPanel, line 65:

    // 'Solution' and 'Views' panels have same width, 'Tools' panel does not
    var xMargin = 15;
    var panelOptions = {
      minWidth: Math.max( solutionControl.width, Math.max( viewsControl.width, toolsControl.width ) ) + ( 2 * xMargin ),
      fill: ABSColors.CONTROL_PANEL_BACKGROUND,
      xMargin: xMargin,
      yMargin: 6,
      align: 'left'
    };
    options.children = [
      new VBox( {
        spacing: TITLE_Y_SPACING,
        align: 'left',
        children: [
          solutionTitle,
          new Panel( solutionControl, panelOptions )
        ]
      } ),
      new VBox( {
        spacing: TITLE_Y_SPACING,
        align: 'left',
        children: [
          viewsTitle,
          new Panel( viewsControl, panelOptions )
        ]
      } ),
      new VBox( {
        spacing: TITLE_Y_SPACING,
        align: 'left',
        children: [
          toolsTitle,
          // Reset All button to right of 'Tools' panel
          new HBox( { children: [ new Panel( toolsControl, panelOptions ), resetAllButton ], spacing: 10 } )
        ]
      } )
    ];

    // stack panels vertically
    VBox.call( this, options );

Which looks like this:

Note that the content of each panel is left aligned, so that all of the radio buttons line up.

If I change to align:'center', then it looks like this, which (based on the examples I provided above) is not what we typically want:

[samreid]

@pixelzoom and I collaborated in the preceding commit to make the alignments match when content width >= minWidth.