Components with "content" should refrain from Decorator Pattern

[marlitas]

Components that handle the layout of their children such as: ButtonNode, AccordionBox, Panel, Carousel, etc should not use the decorator pattern. Adding children to these components outside of the "content" or "contentNode" seems like an anti-pattern. We would like to discourage these Node subclasses from adding children through options or mutation.

Affected ButtonNode Subclasses:

• [x] RectangularRadioButton (sun)
• [x] LevelSelectionButton (vegas)
• [x] SmallStepsLockToggleButton (quadrilateral)

Affected AccordionBox Subclasses:

• [x] EnergyBarGraphAccordionBox (energy-skate-park)
• [x] EnergyGraphAccordionBox (energy-skate-park)
• [x] ShoppingQuestionsAccordionBox (unit-rates)
• [x] PHAccordionBox (ph-scale)

Affected Panel Subclasses/Usages:

• [x] BAAScreenView (build-an-atom, labelVisibilityControlPanel)
• [x] PieChartLegend (energy-skate-park)

[marlitas]

Dev Meeting 12/7/23

JO: Any objections to refactoring to remove these usages?

• No Objections, it will be loud to prevent future usages.

[pixelzoom]

I'll be happy to address refactoring any in the sims that I'm responsible for. I see 2 so far in the list above, which I'll take care of now.

[pixelzoom]

Some other common-code UI components that have "content" (not a complete list) ...

• Checkbox
• Dialog
• Carousel
• KeyboardHelpSection

[marlitas]

Thanks @pixelzoom! I'll keep going through components to make a comprehensive list. I will plan on refactoring the common code components that need this work done.

[pixelzoom]

In case it's useful, I added this at the end of relevant constructors to identify culprits:

    this.childrenChangedEmitter.addListener( () => console.log( new Error( 'children changed' ).stack ) );

[marlitas]

So far none of these components have raised any errors:

• Checkbox
• Dialog
• Carousel
• KeyboardHelpSection

I'll continue checking components that use the "content" pattern, especially those that extend sizable.

[marlitas]

Tested components:

• [x] AccordionBox
• [x] AlignBox
• [x] AquaRadioButton
• [x] AquaRadioButtonGroup
• [x] ButtonNode
• [x] Carousel
• [x] Checkbox
• [x] Dialog
• [x] KeyboardHelpSection
• [x] NumberControl
• [x] Panel
• [x] RichText
• [x] Slider

[marlitas]

This would be good to put on the radar again. I would love some help as I don't think I have the bandwidth to complete myself. I'll add it to the dev board to discuss in the next meeting and see if I can wrangle a co-conspirator.

[jessegreenberg]

Recommendation from developer meeting:

• Prioritize the common code components. Leave sim specific components for the responsible developers. They will need to work on it when it is time for dynamic layout.

I will take a look at the common code components this iteration, and also review the assertions that were proposed here.

It is possible that some of these have already been fixed.

[pixelzoom]

I will take a look at the common code components this iteration, and also review the assertions that were proposed here.

It is possible that some of these have already been fixed.

From https://github.com/phetsims/sun/issues/860#issue-2031266798, the common-code components are:

• [ ] RectangularRadioButton (sun)
• [ ] LevelSelectionButton (vegas)

LevelSelectionButton is already fixed. @marlitas said the problem was fixed in https://github.com/phetsims/vegas/commit/f605de47e2d8c5ede7d12b668d637865cda80e5f. And I recently (7/31/24) added support for dynamic layout.

I don't know about RectangularRadioButton.

[jessegreenberg]

Here is what I have done so far -

• assertNoAdditionalChildren was added to /scenery/js/util/. You can add it to a component after content has been set up.
• RectangularRadioButton and the components listed in https://github.com/phetsims/sun/issues/860#issuecomment-1856551438 are using it and sims have been changed to avoid the assertion. Side issues were assigned to responsible devs for review.

While working on this, it was pointed out that some components should allow having children added to them (like Slider and RichText), because the implementation of their layout does not make assumptions about children.

However, @jonathanolson suggested that all common code components should discourage adding children to them because a) it is difficult to review the implementation of each and decide whether assumptions are made and b) adding children to UI components makes usages less maintainable.

We need to check in at developer meeting to decide next steps.

[jonathanolson]

Dev meeting decision:

• This is a general anti-pattern, components should be responsible for their own children, and outside code shouldn't add children to them.
• We'll want to add assertions to ComboBox and any remaining Sizable components (and potentially everything in sun/scenery-phet as a chip-away)
• We'll want to StrictOmit children from common component options
• Common code Node components should ideally have a run-time assertion for detection + StrictOmit on children.

@jessegreenberg thoughts on what would be good to do now?

[marlitas]

@jessegreenberg I will have some time again to finish this off. Would you like to meet and discuss next steps?