Hide sun panel when its children are all invisible

[samreid]

Address https://github.com/phetsims/sun/issues/824

[samreid]

Good news, everyone! Panels already disappear if you remove their elements. You can demonstrate this in CCK by tapping on a battery, then making the reverse battery button, slider and trash button invisible. The panel automatically disappears.

So why doesn't this work for the DisplayOptionsPanel in the top right? Because its width is managed by an AlignBox. Removing the AlignBox breaks the alignment (now it doesn't match size with lower panel), but now when you remove the radio buttons and checkboxes, the panel disappears:

```diff Subject: [PATCH] Use Node instead of HBox for content layout, see https://github.com/phetsims/circuit-construction-kit-common/issues/927 --- Index: js/view/DisplayOptionsPanel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/view/DisplayOptionsPanel.ts b/js/view/DisplayOptionsPanel.ts --- a/js/view/DisplayOptionsPanel.ts (revision bbb7f7b9fc419425fb1b55a58b4b2533f5c6ba81) +++ b/js/view/DisplayOptionsPanel.ts (date 1675316649021) @@ -148,14 +148,10 @@ ...( showStopwatchCheckbox ? [ stopwatchCheckbox! ] : [] ) ]; - super( alignGroup.createBox( new VBox( { + super( new VBox( { children: children, spacing: SPACING, align: 'left' - } ), { - - // left align within the box - xAlign: 'left' } ), tandem, { yMargin: 10 } ); ```

Is there a way to match their widths while still preserving the "disappear when all content is gone" idea? Perhaps @jonathanolson or @marlitas would know best?

[samreid]

I added a sim-specific workaround which has the desired behavior. I'm not a fan of the complexity, or of having to do this for other panels (in CCK or elsewhere). Hopefully there is a better way? @matthew-blackman or @jonathanolson or @marlitas what do you think?

[jonathanolson]

Why using AlignBox? If it's to have the panels be the same with, just use stretch: true on the VBox?

[samreid]

Good idea, thanks! I will try it.

[marlitas]

I started going down the https://github.com/phetsims/circuit-construction-kit-common/issues/939#issuecomment-1414486082 path with this patch, but quickly remembered conversations about VBox not having a good way to stretch to it's parent container (@jonathanolson please correct me if I'm wrong).

@samreid and I talked through this patch a bit and also came across accordion box still now being a resizable component. https://github.com/phetsims/sun/issues/803 That may be affecting how accordion box is reacting in this patch as well as in https://github.com/phetsims/circuit-construction-kit-common/issues/940.

It seems some designer input might be needed on best way to move forward in terms of effort and priorities.

``` diff Subject: [PATCH] play with stretch --- Index: js/view/DisplayOptionsPanel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/view/DisplayOptionsPanel.ts b/js/view/DisplayOptionsPanel.ts --- a/js/view/DisplayOptionsPanel.ts (revision 03c4a310af6f7289c31bb467fe69c2735dec4f31) +++ b/js/view/DisplayOptionsPanel.ts (date 1675380561253) @@ -40,7 +40,6 @@ public readonly stopwatchCheckbox: CCKCCheckbox | null; /** - * @param alignGroup - box for aligning with other controls * @param showCurrentProperty - true if current should be shown * @param currentTypeProperty - true if current should be shown as electrons or conventional * @param showValuesProperty - true if values should be shown @@ -49,7 +48,7 @@ * @param showStopwatchCheckbox - true if stopwatch should be shown * @param tandem */ - public constructor( alignGroup: AlignGroup, showCurrentProperty: Property, currentTypeProperty: Property, showValuesProperty: Property, showLabelsProperty: Property, + public constructor( showCurrentProperty: Property, currentTypeProperty: Property, showValuesProperty: Property, showLabelsProperty: Property, stopwatch: Stopwatch, showStopwatchCheckbox: boolean, tandem: Tandem ) { const textIconSpacing = 11; @@ -154,11 +153,8 @@ spacing: SPACING, align: 'left' } ); - const alignBox = alignGroup.createBox( vBox, { xAlign: 'left' } ); - const content = new Node( { excludeInvisibleChildrenFromBounds: true } ); - vBox.boundsProperty.link( bounds => content.setChildren( bounds.isValid() ? [ alignBox ] : [] ) ); - super( content, tandem, { + super( vBox, tandem, { yMargin: 10 } ); Index: js/view/CCKCScreenView.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/view/CCKCScreenView.ts b/js/view/CCKCScreenView.ts --- a/js/view/CCKCScreenView.ts (revision 03c4a310af6f7289c31bb467fe69c2735dec4f31) +++ b/js/view/CCKCScreenView.ts (date 1675381445439) @@ -224,7 +224,6 @@ // so that subclasses can add a layout circuit element near it this.sensorToolbox = new SensorToolbox( - CONTROL_PANEL_ALIGN_GROUP, this.circuitNode, voltmeterNodes, ammeterNodes, @@ -248,7 +247,6 @@ } ); this.displayOptionsPanel = new DisplayOptionsPanel( - CONTROL_PANEL_ALIGN_GROUP, model.circuit.showCurrentProperty, model.circuit.currentTypeProperty, model.showValuesProperty, @@ -260,7 +258,6 @@ this.advancedAccordionBox = options.showAdvancedControls ? new AdvancedAccordionBox( model.circuit, - CONTROL_PANEL_ALIGN_GROUP, options.hasACandDCVoltageSources ? sourceResistanceStringProperty : batteryResistanceStringProperty, tandem.createTandem( 'advancedAccordionBox' ), { showRealBulbsCheckbox: !options.hasACandDCVoltageSources @@ -294,6 +291,7 @@ const controlPanelVBox = new VBox( { spacing: VERTICAL_MARGIN, + stretch: true, children: options.showAdvancedControls ? [ this.displayOptionsPanel, this.sensorToolbox, this.advancedAccordionBox! ] : [ this.displayOptionsPanel, this.sensorToolbox ] @@ -303,7 +301,8 @@ xAlign: 'right', yAlign: 'top', xMargin: HORIZONTAL_MARGIN, - yMargin: VERTICAL_MARGIN + yMargin: VERTICAL_MARGIN, + group: CONTROL_PANEL_ALIGN_GROUP } ); this.visibleBoundsProperty.linkAttribute( box, 'alignBounds' ); Index: js/view/AdvancedAccordionBox.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/view/AdvancedAccordionBox.ts b/js/view/AdvancedAccordionBox.ts --- a/js/view/AdvancedAccordionBox.ts (revision 03c4a310af6f7289c31bb467fe69c2735dec4f31) +++ b/js/view/AdvancedAccordionBox.ts (date 1675380561259) @@ -30,12 +30,11 @@ /** * @param circuit - * @param alignGroup - to match the width of other panels * @param batteryResistanceControlString * @param tandem * @param [providedOptions] */ - public constructor( circuit: Circuit, alignGroup: AlignGroup, batteryResistanceControlString: TReadOnlyProperty, tandem: Tandem, providedOptions?: AdvancedAccordionBoxOptions ) { + public constructor( circuit: Circuit, batteryResistanceControlString: TReadOnlyProperty, tandem: Tandem, providedOptions?: AdvancedAccordionBoxOptions ) { const options = optionize()( { showRealBulbsCheckbox: true @@ -47,8 +46,8 @@ }; // Factor out titles const children: Node[] = [ - new WireResistivityControl( circuit.wireResistivityProperty, alignGroup, TEXT_OPTIONS, tandem.createTandem( 'wireResistivityControl' ) ), - new SourceResistanceControl( circuit.sourceResistanceProperty, alignGroup, batteryResistanceControlString, TEXT_OPTIONS, tandem.createTandem( 'sourceResistanceControl' ) ) + new WireResistivityControl( circuit.wireResistivityProperty, TEXT_OPTIONS, tandem.createTandem( 'wireResistivityControl' ) ), + new SourceResistanceControl( circuit.sourceResistanceProperty, batteryResistanceControlString, TEXT_OPTIONS, tandem.createTandem( 'sourceResistanceControl' ) ) ]; if ( options.showRealBulbsCheckbox ) { @@ -61,11 +60,11 @@ } ) ); } - super( alignGroup.createBox( new VBox( { + super( new VBox( { align: 'left', spacing: 15, children: children - } ) ), CircuitConstructionKitCommonStrings.advancedStringProperty, tandem, { + } ), CircuitConstructionKitCommonStrings.advancedStringProperty, tandem, { // Left align the title, with no padding titleAlignX: 'left', Index: js/view/SourceResistanceControl.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/view/SourceResistanceControl.ts b/js/view/SourceResistanceControl.ts --- a/js/view/SourceResistanceControl.ts (revision 03c4a310af6f7289c31bb467fe69c2735dec4f31) +++ b/js/view/SourceResistanceControl.ts (date 1675380561242) @@ -26,12 +26,11 @@ /** * @param sourceResistanceProperty - axon Property for the internal resistance of all Batteries - * @param alignGroup * @param batteryResistanceControlString * @param titleConfig * @param tandem */ - public constructor( sourceResistanceProperty: Property, alignGroup: AlignGroup, + public constructor( sourceResistanceProperty: Property, batteryResistanceControlString: TReadOnlyProperty, titleConfig: TextOptions, tandem: Tandem ) { /** Index: js/view/WireResistivityControl.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/view/WireResistivityControl.ts b/js/view/WireResistivityControl.ts --- a/js/view/WireResistivityControl.ts (revision 03c4a310af6f7289c31bb467fe69c2735dec4f31) +++ b/js/view/WireResistivityControl.ts (date 1675380561265) @@ -27,11 +27,10 @@ /** * @param wireResistivityProperty - * @param alignGroup - for alignment with other controls * @param titleConfig * @param tandem */ - public constructor( wireResistivityProperty: Property, alignGroup: AlignGroup, titleConfig: TextOptions, tandem: Tandem ) { + public constructor( wireResistivityProperty: Property, titleConfig: TextOptions, tandem: Tandem ) { const titleNode = new Text( wireResistivityStringProperty, combineOptions( { tandem: tandem.createTandem( 'titleText' ) }, titleConfig ) ); Index: js/view/SensorToolbox.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/view/SensorToolbox.ts b/js/view/SensorToolbox.ts --- a/js/view/SensorToolbox.ts (revision 03c4a310af6f7289c31bb467fe69c2735dec4f31) +++ b/js/view/SensorToolbox.ts (date 1675380561235) @@ -57,7 +57,6 @@ export default class SensorToolbox extends CCKCPanel { /** - * @param alignGroup - for alignment with other controls * @param circuitNode - the main circuit node to use as a coordinate frame * @param voltmeterNodes - nodes that display the Voltmeters * @param ammeterNodes - nodes that display the Ammeters @@ -66,7 +65,7 @@ * @param tandem * @param [providedOptions] */ - public constructor( alignGroup: AlignGroup, circuitNode: CircuitNode, voltmeterNodes: VoltmeterNode[], + public constructor( circuitNode: CircuitNode, voltmeterNodes: VoltmeterNode[], ammeterNodes: AmmeterNode[], voltageChartNodes: VoltageChartNode[], currentChartNodes: CurrentChartNode[], tandem: Tandem, providedOptions?: SensorToolboxOptions ) { const circuit = circuitNode.circuit; @@ -252,11 +251,11 @@ excludeInvisibleChildrenFromBounds: true } ); - const topBox = alignGroup.createBox( new HBox( { + const topBox = new HBox( { spacing: ( options.showNoncontactAmmeters && options.showSeriesAmmeters ) ? 20 : 40, align: 'bottom', children: [ voltmeterToolNode, ammeterToolNode ] - } ) ); + } ); const rows: Node[] = [ topBox ]; if ( options.showCharts ) { ```

[jonathanolson]

I believe VBox naturally can stretch to the size of a parent container (based on preferred width).

[arouinfar]

Instead of shrinking the AccordionBox width, can we use it as the preferred with of the panel? Basically, let the panel expand to match instead of trying to shrink down the AccordionBox. From @jonathanolson's comment above, that sounds like a possibility.

[samreid]

In the patch, the accordion box is thinner than the above panels, so it looks like this (ignore the fact it is over the carousel):

So we would have to make the accordion box >= the width of the other panels for that strategy to work. AccordionBox does not yet dynamically reshape.

I'm feeling this issue could be turned into a flex "if time" goal and removed from the milestone. Some panels automatically disappear when the last component is removed. Others do not. For this version, clients can just hide the whole panel if needed. We can keep this feature in mind for future phet-io sims when there aren't other layout considerations.

@arouinfar ok to remove from milestone?

[arouinfar]

I'm feeling this issue could be turned into a flex "if time" goal and removed from the milestone. Some panels automatically disappear when the last component is removed. Others do not. For this version, clients can just hide the whole panel if needed. We can keep this feature in mind for future phet-io sims when there aren't other layout considerations.

@arouinfar ok to remove from milestone?

Let's remove it from the milestone. Using a panel's visibleProperty will always be the easiest way to completely hide it. I do not think automatically hiding the panel is a necessary feature.

[samreid]

I feel we could go one step further and just close this issue? It doesn't seem important to implement or have on a back-up list in my opinion. @arouinfar sound OK?

[arouinfar]

Let's close!