Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add displayOnlyProperty to NumberPicker and NumberDisplay #884

Open
amanda-phet opened this issue May 21, 2024 · 1 comment
Open

Add displayOnlyProperty to NumberPicker and NumberDisplay #884

amanda-phet opened this issue May 21, 2024 · 1 comment

Comments

@amanda-phet
Copy link

https://github.com/phetsims/phet-io/issues/1757
#451
#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

  • stroke
  • fill
  • text/number
  • rounded corners
  • units
  • icon/image/graphic

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)

  • numberDisplay
  • numberPicker
  • checkboxes (especially when they serve as a legend)
  • accordion box
  • combobox
  • radio buttons?
  • a/b switch?

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.

@pixelzoom pixelzoom changed the title Add displayOnlyProperty to numberPicker and numberDisplay Add displayOnlyProperty to NumberPicker and NumberDisplay May 21, 2024
@samreid
Copy link
Member

samreid commented May 21, 2024

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

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?

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:

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants