phetsims / sun

User-interface components for PhET simulations, built on top of Scenery.
MIT License
4 stars 12 forks source link

Add displayOnlyProperty to NumberPicker and NumberDisplay #884

Open amanda-phet opened 2 months ago

amanda-phet commented 2 months ago

https://github.com/phetsims/phet-io/issues/1757 https://github.com/phetsims/sun/issues/451 https://github.com/phetsims/sun/issues/617

We would like a displayOnlyProperty` in PhET-iO studio so we can display the value of a number picker without just using enabledProperty. Using enabledProperty can make the values hard to read.

@pixelzoom said: we shouldn't design this based on how we want it to appear in one situation and surgically remove specific components. Instead we should replace it with a preferred component (e.g. a simple box with a stroke, fill, rounded corners), and reuse that wherever we have other components that have displayOnlyProperty.

This would be its own component (called displayPanel for example) that you could choose to use in place of a different component (numberPicker, comboBox, etc.).

@kathy-phet wonders if it could inherit characteristic of the component it's replacing. @pixelzoom says we shouldn't do that.

How much are we trying to optimize the design and user experience?

if we create a displayOnlyProperty for every common code component, need to test for it, account for dynamic layout, etc.

@samreid is wondering if we really need to bake this into the common code property, and instead just define it when needed. We wonder if this will create duplication, inconsistent experiences.

Desired things we can customize for displayOnlyProperty

Which components would benefit from this displayOnlyProperty being used? (Listed a few below, but for now we'd like to just start with number-related components)

Having this ability for numbers seems high value for clients. Not sure about other components.

Can our goals be achieved using enabledProperty? Perhaps the text and/or number doesn't dim, only the interactive controls do? Make sure the value is clearly readable?

@amanda-phet brought up that number pickers are in a LOT of math sims, and having a displayOnlyProperty would be really nice and improve the designer experience when trying to set up problems where we might want only a subset of pickers interactive and others not interactive.

To decide ASAP: is this blocking Mean: Share and Balance? Or should this be addressed in a future sim? If there is a sim on deck that will benefit from this added value we could address it with that sim. We could do a sim-specific implementation for MSaB and not address this in the common code yet.

There is a precedent for addressing something in a future sim when we know we have time for it. @zepumph said this was not an ideal workflow.

For MSaB: We are leaning toward an option in enabledProperty that hides the arrows (feels too much like visibleProperty), or grays out the arrows and not the number value (works in this context, but might not generalize), or darkens the number when setting enabledProperty to false (node is handling this right now, so not sure about this). Decision: Let's make a numberPicker subclass in MSaB that overrides numberPicker and see what problems we run into.

Bottom line: we are spending a lot of time over-designing features that we don't have clients asking for yet, but we are going to move forward anyway because we have designers asking for this.

samreid commented 2 months ago

Some patches from today's meeting demos:

Sim-specific code

The simulation instantiates a ToggleNode to swap between the control and the alternate display.

PRO: The alternate display is fully customizable (not limited to predetermined customizations in common code) PRO: The alternate display is extrinsic to the component, thus avoiding complication of the component itself CON: Code duplication CON: Reliance on convention to match tandems, tree structure, and behavior

```diff Subject: [PATCH] Adjust comments, mark fields as readonly, change spelling of ochre, see https://github.com/phetsims/density-buoyancy-common/issues/123 --- Index: js/common/view/TablePlateNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/view/TablePlateNode.ts b/js/common/view/TablePlateNode.ts --- a/js/common/view/TablePlateNode.ts (revision 37cfc7f367c5bee9eb991ab8bf067077cfc85537) +++ b/js/common/view/TablePlateNode.ts (date 1716323201932) @@ -8,7 +8,7 @@ * @author John Blanco (PhET Interactive Simulations) */ -import { Image, Node, NodeOptions } from '../../../../scenery/js/imports.js'; +import { Image, Node, NodeOptions, Text } from '../../../../scenery/js/imports.js'; import meanShareAndBalance from '../../meanShareAndBalance.js'; import Plate from '../model/Plate.js'; import NumberPicker from '../../../../sun/js/NumberPicker.js'; @@ -28,6 +28,10 @@ import SnackQuantitySoundPlayer, { SnackQuantitySoundPlayerOptions } from './SnackQuantitySoundPlayer.js'; import soundManager from '../../../../tambo/js/soundManager.js'; import optionize from '../../../../phet-core/js/optionize.js'; +import BooleanProperty from '../../../../axon/js/BooleanProperty.js'; +import BooleanToggleNode from '../../../../sun/js/BooleanToggleNode.js'; +import DerivedProperty from '../../../../axon/js/DerivedProperty.js'; +import Panel from '../../../../sun/js/Panel.js'; type SelfOptions = { snackType: SnackType; @@ -67,17 +71,23 @@ MeanShareAndBalanceConstants.MIN_NUMBER_OF_SNACKS_PER_PLATE, MeanShareAndBalanceConstants.MAX_NUMBER_OF_SNACKS_PER_PLATE ); - const numberPicker = new NumberPicker( - plate.tableSnackNumberProperty, - new Property( numberPickerRange ), - { - centerTop: new Vector2( plateImage.centerBottom.x, 30 ), + + const displayOnlyProperty = new BooleanProperty( false, { + tandem: options.tandem.createTandem( 'displayOnlyProperty' ) + } ); + const numberPicker = new NumberPicker( plate.tableSnackNumberProperty, new Property( numberPickerRange ), { color: MeanShareAndBalanceColors.numberPickerColorProperty, valueChangedSoundPlayer: snackQuantitySoundPlayer, boundarySoundPlayer: snackQuantitySoundPlayer, tandem: options.tandem.createTandem( 'numberPicker' ) } ); + const toggleNode = new BooleanToggleNode( displayOnlyProperty, + new Panel( new Text( new DerivedProperty( [ plate.tableSnackNumberProperty ], tableSnack => tableSnack.toFixed( 0 ) ), { fontSize: 25 } ) ), + numberPicker, { + centerTop: new Vector2( plateImage.centerBottom.x, 30 ) + } + ); // Create and position the Nodes representing the individual snacks that are on this plate. const snacks = _.times( @@ -119,7 +129,7 @@ } ); super( { - children: [ plateAndSnacksNode, numberPicker ], + children: [ plateAndSnacksNode, toggleNode ], centerX: plate.xPositionProperty.value, visibleProperty: plate.isActiveProperty } ); ```
image

Disabled component = Opaque Number

In this patch, the number is left opaque when disabled:

PRO: Reuse/redefine enabledProperty instead of adding a separate Property PRO: Very lightweight CON: Do we always want arrowheads? CON: Would this strategy work in other components? CON: Would it be inconsistent for some of our components to be grayed out when disabled and others not be fully grayed out?

```diff Subject: [PATCH] Adjust comments, mark fields as readonly, change spelling of ochre, see https://github.com/phetsims/density-buoyancy-common/issues/123 --- Index: js/NumberPicker.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/NumberPicker.ts b/js/NumberPicker.ts --- a/js/NumberPicker.ts (revision 3cd5c702159e13688afd1b0b9da468f129ccee71) +++ b/js/NumberPicker.ts (date 1716325790610) @@ -164,7 +164,7 @@ onInput: _.noop, incrementEnabledFunction: ( value: number, range: Range ) => ( value !== null && value !== undefined && value < range.max ), decrementEnabledFunction: ( value: number, range: Range ) => ( value !== null && value !== undefined && value > range.min ), - disabledOpacity: SceneryConstants.DISABLED_OPACITY, + disabledOpacity: 1, valueChangedSoundPlayer: generalSoftClickSoundPlayer, boundarySoundPlayer: generalBoundaryBoopSoundPlayer, @@ -321,7 +321,9 @@ const arrowOptions = { stroke: options.arrowStroke, lineWidth: options.arrowLineWidth, - pickable: false + pickable: false, + enabledProperty: this.enabledProperty, + disabledOpacity: 0.1 }; // increment arrow, pointing up, described clockwise from tip ```
image

UPDATE FROM MK: I think this is a slightly better patch for the enabled + black number change:

```diff Subject: [PATCH] update doc, https://github.com/phetsims/buoyancy/issues/64 --- Index: js/NumberPicker.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/NumberPicker.ts b/js/NumberPicker.ts --- a/js/NumberPicker.ts (revision 3cd5c702159e13688afd1b0b9da468f129ccee71) +++ b/js/NumberPicker.ts (date 1716333893139) @@ -164,7 +164,7 @@ onInput: _.noop, incrementEnabledFunction: ( value: number, range: Range ) => ( value !== null && value !== undefined && value < range.max ), decrementEnabledFunction: ( value: number, range: Range ) => ( value !== null && value !== undefined && value > range.min ), - disabledOpacity: SceneryConstants.DISABLED_OPACITY, + disabledOpacity: 1, valueChangedSoundPlayer: generalSoftClickSoundPlayer, boundarySoundPlayer: generalBoundaryBoopSoundPlayer, @@ -312,7 +312,9 @@ const strokedBackground = new Rectangle( 0, 0, backgroundWidth, backgroundHeight, backgroundCornerRadius, backgroundCornerRadius, { pickable: false, stroke: options.backgroundStroke, - lineWidth: options.backgroundLineWidth + lineWidth: options.backgroundLineWidth, + disabledOpacity: SceneryConstants.DISABLED_OPACITY, + enabledProperty: this.enabledProperty } ); // compute size of arrows @@ -321,7 +323,9 @@ const arrowOptions = { stroke: options.arrowStroke, lineWidth: options.arrowLineWidth, - pickable: false + pickable: false, + disabledOpacity: SceneryConstants.DISABLED_OPACITY, + enabledProperty: this.enabledProperty }; // increment arrow, pointing up, described clockwise from tip ```