phetsims / sun

User-interface components for PhET simulations, built on top of Scenery.
MIT License
4 stars 12 forks source link

Buggy layout using ComboBox with layoutNode option #788

Closed pixelzoom closed 2 years ago

pixelzoom commented 2 years ago

To demonstrate, make this change in BLL SolutePanel.ts:

-      children: [ solutionComboBox, toggleNode ]
+      children: [ solutionComboBox ]

The result looks like this:

screenshot_1862

I'm guessing that the optional labelNode was not considered when resize behavior was implemented for ComboBox, and it's assuming that the ComboBoxButton designes the size.

This is impacting https://github.com/phetsims/beers-law-lab/issues/288, dynamic layout for BLL.

pixelzoom commented 2 years ago

ComboBox is also not adjusting it's layout when labelNode changes size. This bit of code probably needs to be replaced with an HBox:

    if ( options.labelNode ) {
      this.button.left = options.labelNode.right + options.labelXSpacing;
      this.button.centerY = options.labelNode.centerY;
    }
pixelzoom commented 2 years ago

To see this in BLL, you can look at the label + ComboBox in either screen. Here's the Solute ComboBox in the Concentration screen (see SolutePanel.ts). Note (1) the extra space between the label and the ComboBox, (2) the incorrect ComboBox resize behavior, causing it's parent VBox to resize incorrectly, and (3) the partially drawn ComboBox.

screenshot_1865
jonathanolson commented 2 years ago

Confirmed that it's an oversight of mine in ComboBox layout, it's not respecting preferred sizes correctly with a labelNode. I should have this fixed up today.

jonathanolson commented 2 years ago

@marlitas and I fixed this in the above commit. @pixelzoom can you verify? Apologies about that.

pixelzoom commented 2 years ago

No apologies necessary. Looks and behaves great in BLL. Thanks for the quick turnaround. Closing.