search

phetsims / sun

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

Sun buttons do not resize correctly when their content has dynamic bounds. #781

Closed samreid closed 2 years ago

samreid commented 2 years ago

Discovered in https://github.com/phetsims/chipper/issues/1302 by @jonathanolson and reviewed by @samreid and @zepumph .

zepumph commented 2 years ago

As part of this issue. It would be helpful if you could explain why the button didn't work, but a nested VBox did. I don't really understand that part.

samreid commented 2 years ago

In slack @jonathanolson said:

widthSizable: true

zepumph commented 2 years ago

I'm pretty sure this issue is just a duplicate of https://github.com/phetsims/sun/issues/783

zepumph commented 2 years ago

Oops, didn't mean to close.

pixelzoom commented 2 years ago

Not a duplicate of #783. This is a problem that is specific to #783. And the resize behavior of sun buttons in general (not just push buttons that involve Text is incorrect, as reported in https://github.com/phetsims/sun/issues/783#issuecomment-1227843745. So I'm going to generalize the title of this issue to reflect that.

jonathanolson commented 2 years ago

Handled above. @samreid can you confirm?

jessegreenberg commented 2 years ago

I am seeing some layout regressions in sun buttons and thought it could be related to this issue. For example

Radio buttons in moleculesl-and-light ![image](https://user-images.githubusercontent.com/6396244/187776864-4c0c6263-7191-4aa0-88cc-2db39d61c9b7.png)
Radio buttons in function-builder ![image](https://user-images.githubusercontent.com/6396244/187776966-c5595591-1919-4483-a3a5-57751a33040f.png)
Carousel buttons ![image](https://user-images.githubusercontent.com/6396244/187777046-826c69f8-1ef9-42fe-9b26-aec9eac7077c.png)

There may be more, but those were some examples I noticed.

samreid commented 2 years ago

@jonathanolson can you please take the lead on the preceding comment before I review?

jonathanolson commented 2 years ago

There was a flow for buttons setting specific sizes that wasn't handled, that should be fixed with the above commit.

pixelzoom commented 2 years ago

Reviewed behavior in the context of Natural Selection, and button resize behavior is now as expected. I did not review the code changes. Closing.