search

phetsims / molecule-shapes

"Molecule Shapes" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/molecule-shapes
GNU General Public License v3.0
5 stars 6 forks source link

ComboBox width does not appropriately scale #225

Closed arouinfar closed 1 year ago

arouinfar commented 1 year ago

Related to #223

When the Molecule panel expands to maintain width with the Options panel, the ComboBox ends up oddly stretched. The button is overly large and the ListBox width also appears to be too narrow.

image

arouinfar commented 1 year ago

Tagging for https://github.com/phetsims/qa/issues/844 and https://github.com/phetsims/qa/issues/845.

KatieWoe commented 1 year ago

Noting that this did not occur in the published version of the sim. Dev version: now Published version: then

samreid commented 1 year ago

I'll take a look...

samreid commented 1 year ago

We can shut off the resizing of the combo box like so:

image

It wasn't clear from the issue comments whether that is the desired solution. @arouinfar or @KatieWoe can you please review and close if all is well?

KatieWoe commented 1 year ago

I'm not seeing the molecule box expand in master so I'm not sure how to check for this at the moment.

notlonger
samreid commented 1 year ago

@arouinfar does this look ok to you? (No longer stretching the combo box?)

arouinfar commented 1 year ago

@samreid looks good to me.

I'm not seeing the molecule box expand in master so I'm not sure how to check for this at the moment.

Since the ComboBox contents (chemical formulas) aren't translatable and #224 hasn't been resolved yet, I tested it in a roundabout way. Increasing the width of a checkbox label will cause the Options panel (and therefore the Molecule panel) to expand.

image
Nancy-Salpepi commented 1 year ago

This issue still exists in https://github.com/phetsims/qa/issues/905. Reopening.

Screenshot 2023-02-21 at 3 56 03 PM
jonathanolson commented 1 year ago

Weirdly enough, I think my fix for https://github.com/phetsims/molecule-shapes/issues/242 also applies here (handles resizing of content in those "titled" panels). I can't reproduce this in master. @Nancy-Salpepi are you able to reproduce this in master?

Nancy-Salpepi commented 1 year ago

This looks good in master now, with the fix from https://github.com/phetsims/molecule-shapes/issues/242 (even though that issue is still present)

jonathanolson commented 1 year ago

Please close after verifying!

Nancy-Salpepi commented 1 year ago

This is fixed in the PhET brand sim and in a Standard wrapper, but is broken in the Studio wrapper. The combobox doesn't shift when the panel resizes as I change locales.

Screenshot 2023-03-02 at 8 43 55 PM
jonathanolson commented 1 year ago

I believe this is fixed in master. @Nancy-Salpepi can you verify?

@zepumph can you complete review of https://github.com/phetsims/sun/commit/b7f1be274e36b1430cc9b55f5611197f31b7a1e0 and https://github.com/phetsims/scenery/commit/cfa90b858bfdfdfb11b3221b36a8ad4cf6076082?

Also leaving assigned to myself for creation of unit tests (there's a lot to verify, and I haven't tested cases where one node is an ancestor of another).

Nancy-Salpepi commented 1 year ago

This looks good in master!

zepumph commented 1 year ago

Looks great. Thanks @jonathanolson! Comments are inlined above.

jonathanolson commented 1 year ago

would it be more performant for below opperations if these were Sets?

I don't believe it would be significantly so at all. This also isn't written with being highly performant for node changes (it already requires a lot of machinery, and I'm trying to keep it simple and readable), so a lot of the closures that are created are probably higher overhead.

KatieWoe commented 1 year ago

Looks good in rc.3