Closed jonathanolson closed 1 month ago
Implemented above. @marlitas can you review?
I'm finishing up Calculus Grapher review today, but can hit this tomorrow.
I was a bit confused by this comment:
// For now just position content. Future updates could include widthResizable content?
is the content not widthResizable as long as a widthResizable node is passed in? Or is this referring to requiring a widthResizable node? Perhaps adding in margins? Mostly curious.
I also have a patch here that renames content
in AquaRadioButtonConstraint
to labelNode
. I wasn't sure why there was a naming shift moving from AquaRadioButton
to the constraint, and was confused at first that content
included the radio button as well as the label, not just the label.
Back to @jonathanolson!
I was a bit confused by this comment
It was a bad comment!
Also applied the naming changes. Thanks for the review!
AquaRadioButton and AquaRadioButtonGroup should be brought up-to-speed for dynamic layout, looks like this is causing https://github.com/phetsims/sun/issues/832.
AquaRadioButton should behave a lot like Checkbox: it's a layout container with the label as its primary content. CheckboxConstraint is almost exactly what we need for AquaRadioButton's layout (perhaps we could factor this out...). We'll need to do a similar "is the mouse/touch area customized" logic presumably for backwards compatibility (unfortunate).
AquaRadioButton should probably also have its own
touchAreaXDilation
et al., similar to other buttons.AquaRadioButtonGroup should just FlowBox them, and if it's a vertical one,
{ stretch: true }
things. It should forward touch/mouse area dilations (instead of setting them, even if setting once).AquaRadioButton is also doing some... almost-DAG-like things by having two parents for the labelNode. AquaRadioButton is ALSO not removing the parents of the label, so it's memory-leaking parents on the label node.
@marlitas, this looks like something either of us could do, thoughts? Looks blocking for https://github.com/phetsims/sun/issues/832, but I don't see RPAL on the current iteration schedule.