Remove `toggleNode.elements` nesting from BlocksValueControlPanel

arouinfar commented 2 weeks ago

In https://github.com/phetsims/buoyancy/issues/51 the devs asked:

On all Compare screens: Do we want to keep the toggleNode.elements under BlocksValueControlPanel? It introduces two levels of nesting.

Here's the current tree structure:

During today's design meeting with @DianaTavares @samreid @AgustinVallejo @zepumph we decided to remove the toggleNode.elements nesting from the tree, so we would see blocksValueControlPanel.volumeNumberControl, etc. instead.

Other notes:

ToggleNode was not originally used in Density, but using this common code element simplified the implementation and is a worthwhile change.
In Density 1.1, the NumberControl related to each scene had its own visibleProperty. Clients could hide the control in the 'Same Density' scene, while leaving them visible in other scenes. This is not supported with the current implementation, and would be a non-trivial change. This customization is not central to the learning goals, so we decided to wait for a client to ask for it.

zepumph commented 2 weeks ago

[x] We can use a workaround like this to get around needing "toggleNode" like this:


Subject: [PATCH] more updates for es (remove unneeded deprecated string), https://github.com/phetsims/babel/issues/24
---
Index: js/common/view/BlocksValueControlPanel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/BlocksValueControlPanel.ts b/js/common/view/BlocksValueControlPanel.ts
--- a/js/common/view/BlocksValueControlPanel.ts (revision b9a38e8df35c2625658ef931fad645e8c7389ddc)
+++ b/js/common/view/BlocksValueControlPanel.ts (date 1720455579267)
@@ -59,7 +59,7 @@
     DensityBuoyancyCommonStrings.massStringProperty,
     DensityBuoyancyCommonStrings.kilogramsPatternStringProperty,
     'kilograms', {

tandem: tandem,
tandem: options.tandem.createTandem( 'massNumberControl' ), sliderOptions: { phetioLinkedProperty: massProperty, trackSize: options.sliderTrackSize, @@ -102,10 +102,7 @@ phetioVisiblePropertyInstrumented: false } )
} ], {
tandem: options.tandem.createTandem( 'toggleNode' ),
phetioVisiblePropertyInstrumented: false
} );
} ] );

super( toggleNode, options ); }
[x] In ToggleNode, let's delete the extra "elements" part generally. This is more nesting than needed, and we don't see any usages of it in other published PhET-iO sims.

zepumph commented 2 weeks ago

We thought about trying to keep ToggleNode instrumented to provide the tandems "correctly" through the group items paradigm, but we would rather not have toggleNode in the studio tree. Thus we did more of a workaround, and opened https://github.com/phetsims/sun/issues/888 about having an easier option to instrument important children without instrumenting a common code parent (like ToggleNode).

samreid commented 2 weeks ago

After this change, the migration wrapper is showing these changes:

{
    "phetioIDNotInNewAPI": [
        "density.compareScreen.view.blocksValueControlPanel.toggleNode.elements.massNumberControl.numberDisplay.visibleProperty",
        "density.compareScreen.view.blocksValueControlPanel.toggleNode.elements.massNumberControl.slider.thumbNode.visibleProperty",
        "density.compareScreen.view.blocksValueControlPanel.toggleNode.elements.massNumberControl.slider.enabledRangeProperty",
        "density.compareScreen.view.blocksValueControlPanel.toggleNode.elements.massNumberControl.slider.visibleProperty",
        "density.compareScreen.view.blocksValueControlPanel.toggleNode.elements.massNumberControl.slider.enabledProperty",
        "density.compareScreen.view.blocksValueControlPanel.toggleNode.elements.massNumberControl.slider.trackNode.visibleProperty",
        "density.compareScreen.view.blocksValueControlPanel.toggleNode.elements.massNumberControl.decrementButton.enabledProperty",
        "density.compareScreen.view.blocksValueControlPanel.toggleNode.elements.massNumberControl.decrementButton.visibleProperty",
        "density.compareScreen.view.blocksValueControlPanel.toggleNode.elements.massNumberControl.incrementButton.enabledProperty",
        "density.compareScreen.view.blocksValueControlPanel.toggleNode.elements.massNumberControl.incrementButton.visibleProperty",
        "density.compareScreen.view.blocksValueControlPanel.toggleNode.elements.massNumberControl.enabledProperty",
        "density.compareScreen.view.blocksValueControlPanel.toggleNode.elements.volumeNumberControl.numberDisplay.visibleProperty",
        "density.compareScreen.view.blocksValueControlPanel.toggleNode.elements.volumeNumberControl.slider.thumbNode.visibleProperty",
        "density.compareScreen.view.blocksValueControlPanel.toggleNode.elements.volumeNumberControl.slider.enabledRangeProperty",
        "density.compareScreen.view.blocksValueControlPanel.toggleNode.elements.volumeNumberControl.slider.visibleProperty",
        "density.compareScreen.view.blocksValueControlPanel.toggleNode.elements.volumeNumberControl.slider.enabledProperty",
        "density.compareScreen.view.blocksValueControlPanel.toggleNode.elements.volumeNumberControl.slider.trackNode.visibleProperty",
        "density.compareScreen.view.blocksValueControlPanel.toggleNode.elements.volumeNumberControl.decrementButton.enabledProperty",
        "density.compareScreen.view.blocksValueControlPanel.toggleNode.elements.volumeNumberControl.decrementButton.visibleProperty",
        "density.compareScreen.view.blocksValueControlPanel.toggleNode.elements.volumeNumberControl.incrementButton.enabledProperty",
        "density.compareScreen.view.blocksValueControlPanel.toggleNode.elements.volumeNumberControl.incrementButton.visibleProperty",
        "density.compareScreen.view.blocksValueControlPanel.toggleNode.elements.volumeNumberControl.enabledProperty",
        "density.compareScreen.view.blocksValueControlPanel.toggleNode.elements.densityNumberControl.numberDisplay.visibleProperty",
        "density.compareScreen.view.blocksValueControlPanel.toggleNode.elements.densityNumberControl.slider.thumbNode.visibleProperty",
        "density.compareScreen.view.blocksValueControlPanel.toggleNode.elements.densityNumberControl.slider.enabledRangeProperty",
        "density.compareScreen.view.blocksValueControlPanel.toggleNode.elements.densityNumberControl.slider.visibleProperty",
        "density.compareScreen.view.blocksValueControlPanel.toggleNode.elements.densityNumberControl.slider.enabledProperty",
        "density.compareScreen.view.blocksValueControlPanel.toggleNode.elements.densityNumberControl.slider.trackNode.visibleProperty",
        "density.compareScreen.view.blocksValueControlPanel.toggleNode.elements.densityNumberControl.decrementButton.enabledProperty",
        "density.compareScreen.view.blocksValueControlPanel.toggleNode.elements.densityNumberControl.decrementButton.visibleProperty",
        "density.compareScreen.view.blocksValueControlPanel.toggleNode.elements.densityNumberControl.incrementButton.enabledProperty",
        "density.compareScreen.view.blocksValueControlPanel.toggleNode.elements.densityNumberControl.incrementButton.visibleProperty",
        "density.compareScreen.view.blocksValueControlPanel.toggleNode.elements.densityNumberControl.enabledProperty"
    ]
}

samreid commented 2 weeks ago

I was going to commit the updated API file and start adding migration processors, but thought I should double check first. Was it intentional to uninstrument all those descendant trees?

zepumph commented 1 week ago

I added a migration rule with @samreid. Anything else here?

phetsims / density-buoyancy-common

Remove `toggleNode.elements` nesting from BlocksValueControlPanel #251