phetsims / sun

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

`RectangularRadioButtonGroup` doesn't update layout when button widths change #852

Closed jbphet closed 4 months ago

jbphet commented 11 months ago

The widths of the buttons in a RectangularRadioButtonGroup don't appear to update when the size of one button changes in a different way from the others. This was first reported as part of https://github.com/phetsims/greenhouse-effect/issues/349, and there is another issue that was created for updating the just the highlight.

I read through the code in RectangularRadioButtonGroup and found a portion of the code that looks like it is intended to make the widths of all the buttons the same. This code currently starts on line 181 of the source file and looks like this:

    // Calculate the maximum width and height of the content, so we can make all radio buttons the same size.
    const widestContentWidth = _.maxBy( nodes, node => node.width )!.width;
    const tallestContentHeight = _.maxBy( nodes, node => node.height )!.height;

This code probably needs to be re-run when the width changes for one or more of the buttons.

jbphet commented 11 months ago

Assigning to the original author, he can pass it on to the dynamic layout experts if necessary.

jbphet commented 11 months ago

I just realized a screenshot would be helpful, so here is an example that can be easily duplicated with the recent Greenhouse Effect dev test version. To duplicate:

  1. Go to https://phet-dev.colorado.edu/html/greenhouse-effect/1.2.0-dev.2/phet/greenhouse-effect_en_phet.html?stringTest=dynamic
  2. Select the first screen (Waves), and then select the "date mode" by pressing the button with the calendar icon "Greenhouse Gas Concentration" control panel
  3. Press the right arrow button to increase the string sizes

image

pixelzoom commented 11 months ago

Aaron Davis was the original author of RectangularRadioButtonGroup. I made changes at various points to manage chaos as this class (and its supporting classes) evolved. I don't see any commits that indicate support for dynamic layout has been added. @jonathanolson has generally been handling dynamic layout for sun components, and it would be good to keep things consistent, so reassigning to him.

marlitas commented 11 months ago

Just to confirm, because I believe I was initially confused when looking at this issue. We want the radio button widths to match across all the buttons? The group is updating is minimumWidths as I would expect, however visually the buttons should all be the same size and that is currently not happening?

I will assign and take a look at this as well.

marlitas commented 5 months ago

@jonathanolson We would like you to prioritize this issue since it is now blocking Greenhouse: https://github.com/phetsims/sun/issues/851

jonathanolson commented 4 months ago

Should be fixed in https://github.com/phetsims/sun/commit/3e553891e497f4af8d93780277e3c6104e6139eb.

@jbphet can you confirm?

jbphet commented 4 months ago

Wow - nice one-line fix @jonathanolson! In fact, there was a significant reduction in code including all the related commits shown above.

I've verified that it's working correctly in Greenhouse Effect for the case described above. I think we're good to go here. Closing.