phetsims / gravity-and-orbits

"Gravity And Orbits" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
12 stars 6 forks source link

Use HSeparator instead of HSeparatorDeprecated #468

Closed samreid closed 1 year ago

samreid commented 1 year ago

So they will autocollapse, see https://github.com/phetsims/scenery/issues/1480

samreid commented 1 year ago

Fixed, and I observed that they correctly collapse in studio.

samreid commented 1 year ago

I also added migration rules so 1.5 will still load. @arouinfar want to review this issue in studio?

arouinfar commented 1 year ago

@samreid the separators disappear as expected, so that's looking good. However, there are a few issues:

samreid commented 1 year ago

The separators extend beyond the bounds of the control panel

I fixed that in the commits.

Setting sceneControl.visibleProperty to false centers the remaining contents creating an odd indentation effect.

This is controlled by the AlignBox in the outer panel. When I change it like so:

```diff Index: js/common/view/GravityAndOrbitsScreenView.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/view/GravityAndOrbitsScreenView.ts b/js/common/view/GravityAndOrbitsScreenView.ts --- a/js/common/view/GravityAndOrbitsScreenView.ts (revision ec65d67b845667482590271d9ef113387e746596) +++ b/js/common/view/GravityAndOrbitsScreenView.ts (date 1667859736948) @@ -77,13 +77,17 @@ right: this.layoutBounds.right - MARGIN, spacing: MARGIN, children: [ - new Panel( alignGroup.createBox( controlPanel ), merge( {}, GravityAndOrbitsConstants.CONTROL_PANEL_OPTIONS, { + new Panel( alignGroup.createBox( controlPanel, { + xAlign: 'left' + } ), merge( {}, GravityAndOrbitsConstants.CONTROL_PANEL_OPTIONS, { tandem: controlPanelTandem, visiblePropertyOptions: { phetioReadOnly: false } } ) ), - new Panel( alignGroup.createBox( massesControlPanel ), merge( {}, GravityAndOrbitsConstants.CONTROL_PANEL_OPTIONS, { + new Panel( alignGroup.createBox( massesControlPanel, { + xAlign: 'left' + } ), merge( {}, GravityAndOrbitsConstants.CONTROL_PANEL_OPTIONS, { tandem: massesControlPanelTandem, visiblePropertyOptions: { phetioReadOnly: false ```

The HSeparator doesn't fill the width.

image

@marlitas or @jonathanolson can you please comment how the GravityAndOrbitsControls can fill to fit its container bounds, while keeping its contents left-aligned?

marlitas commented 1 year ago

Did some initial research on this, and I would assume that passing a stretch: true to GravityAndOrbitsControl would solve the problem. However, even with stretch: true the VBox is calculating it's width to be about 65 px less than the parent AlignBox's width.

This screenshot shows the behavior with scenery helper outlines. The red dash represents the AlignBox bounds.

Screenshot 2022-11-07 at 6 00 04 PM

I have a couple of hypotheses for what might be going on here:

This seems like buggy behavior to me, and something that @jonathanolson and I should look into on scenery's end.

jonathanolson commented 1 year ago

AlignGroup doesn't currently set preferred sizes for anything, and this really isn't a good place to use AlignGroup. The AlignBox is just padding out the content. https://github.com/phetsims/scenery/issues/1493 exists to look into whether AlignGroup should be improved to do this type of thing.

Instead, just use the VBox itself and toss the AlignBoxes/AlignGroups for a simpler approach:

```diff Index: js/common/view/GravityAndOrbitsScreenView.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/view/GravityAndOrbitsScreenView.ts b/js/common/view/GravityAndOrbitsScreenView.ts --- a/js/common/view/GravityAndOrbitsScreenView.ts (revision ec65d67b845667482590271d9ef113387e746596) +++ b/js/common/view/GravityAndOrbitsScreenView.ts (date 1667924635611) @@ -9,10 +9,9 @@ import Vector2 from '../../../../dot/js/Vector2.js'; import ScreenView from '../../../../joist/js/ScreenView.js'; -import merge from '../../../../phet-core/js/merge.js'; import ResetAllButton from '../../../../scenery-phet/js/buttons/ResetAllButton.js'; -import { AlignGroup, Node, VBox } from '../../../../scenery/js/imports.js'; -import Panel from '../../../../sun/js/Panel.js'; +import { Node, VBox } from '../../../../scenery/js/imports.js'; +import Panel, { PanelOptions } from '../../../../sun/js/Panel.js'; import gravityAndOrbits from '../../gravityAndOrbits.js'; import GravityAndOrbitsConstants from '../GravityAndOrbitsConstants.js'; import GravityAndOrbitsControls from './GravityAndOrbitsControls.js'; @@ -20,6 +19,7 @@ import MassControlPanel from './MassControlPanel.js'; import GravityAndOrbitsModel from '../model/GravityAndOrbitsModel.js'; import Tandem from '../../../../tandem/js/Tandem.js'; +import { combineOptions } from '../../../../phet-core/js/optionize.js'; // constants const MARGIN = 5; @@ -35,10 +35,6 @@ tandem: tandem } ); - const alignGroup = new AlignGroup( { - matchVertical: false - } ); - // Control panel in the upper right of the play area. const controlPanelTandem = tandem.createTandem( 'controlPanel' ); const controlPanel = new GravityAndOrbitsControls( model, { @@ -76,18 +72,21 @@ top: this.layoutBounds.top + MARGIN, right: this.layoutBounds.right - MARGIN, spacing: MARGIN, + stretch: true, children: [ - new Panel( alignGroup.createBox( controlPanel ), merge( {}, GravityAndOrbitsConstants.CONTROL_PANEL_OPTIONS, { + new Panel( controlPanel, combineOptions( {}, GravityAndOrbitsConstants.CONTROL_PANEL_OPTIONS, { tandem: controlPanelTandem, visiblePropertyOptions: { phetioReadOnly: false - } + }, + align: 'left' } ) ), - new Panel( alignGroup.createBox( massesControlPanel ), merge( {}, GravityAndOrbitsConstants.CONTROL_PANEL_OPTIONS, { + new Panel( massesControlPanel, combineOptions( {}, GravityAndOrbitsConstants.CONTROL_PANEL_OPTIONS, { tandem: massesControlPanelTandem, visiblePropertyOptions: { phetioReadOnly: false - } + }, + align: 'left' } ) ) ] } ) ); ```
image
samreid commented 1 year ago

Thanks, looks great. @zepumph and I applied the patch and tested. Closing.