Could the pH scale indicator box be smaller when numberDisplay.visibleProperty is false?

[Nancy-Salpepi]

Test device MacBook Air (m1 chip)

Operating System 13.1

Browser safari/chrome/FF

Problem description For https://github.com/phetsims/qa/issues/872 and https://github.com/phetsims/qa/issues/873, when setting macroScreen.view.pHMeterNode.pHIndicatorNode.numberDisplay.visibleProperty to false, the pH "box" looks unnecessarily big.

From slack: Nancy Salpepi 11:54 AM This is my LAST pH scale question!!!!! If I set phScaleBasics.macroScreen.view.pHMeterNode.pHIndicatorNode.numberDisplay.visibleProperty to false, should the box get smaller? Amy Rouinfar 12:00 PM Oh good find. Yeah, it should probably get smaller. 12:01 But I also think it's a somewhat silly customization 12:02 I guess the thought is that clients may want students to read the value from the scale instead of the display

Nancy Salpepi 12:02 PM that is what i was thinking it was for…..should I make an issue about it or let it go?

Amy Rouinfar 12:03 PM Please make an issue 12:04 I think it'd be an improvement and it's probably an easy fix.

[pixelzoom]

... I think it'd be an improvement and it's probably an easy fix.

FYI, not an easy fix. Probably 30-45 minutes to make the background rectangle resize, or to totally reimplement it as something that automatically resizes (probably a Panel with VBox content). And since this is in the Macro screen, whatever I do will need to be patched into both ph-scale 1.6 and ph-scale-basics 1.6, which is another 15-20 minutes.

@arouinfar do you still want me to proceed?

[pixelzoom]

I spent ~1 hour on this tonight. My work is saved in the patch below. I didn't commit because (a) I have many questions, and (b) I want to do this in 1 commit that can be cherry-picked.

@arouinfar please contact me on Slack to discuss at your earliest convenience.

Patch

```diff Subject: [PATCH] make MacroPHMeterNode indicator resize if the NumberDisplay is hidden, https://github.com/phetsims/ph-scale/issues/272 --- Index: js/macro/view/MacroPHMeterNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/macro/view/MacroPHMeterNode.ts b/js/macro/view/MacroPHMeterNode.ts --- a/js/macro/view/MacroPHMeterNode.ts (revision e48561d459954db133ed343cc018963f2b8b2834) +++ b/js/macro/view/MacroPHMeterNode.ts (date 1674266810598) @@ -28,7 +28,7 @@ import NumberDisplay from '../../../../scenery-phet/js/NumberDisplay.js'; import PhetFont from '../../../../scenery-phet/js/PhetFont.js'; import ProbeNode, { ProbeNodeOptions } from '../../../../scenery-phet/js/ProbeNode.js'; -import { DragListener, InteractiveHighlighting, KeyboardDragListener, Line, LinearGradient, Node, NodeOptions, Path, Rectangle, Text } from '../../../../scenery/js/imports.js'; +import { DragListener, InteractiveHighlighting, KeyboardDragListener, Line, LinearGradient, Node, NodeOptions, Path, Rectangle, Text, VBox } from '../../../../scenery/js/imports.js'; import Dropper from '../../common/model/Dropper.js'; import Water from '../../common/model/Water.js'; import PHScaleColors from '../../common/PHScaleColors.js'; @@ -39,17 +39,23 @@ import Solution from '../../common/model/Solution.js'; import { PHValue } from '../../common/model/PHModel.js'; import PHMovable from '../../common/model/PHMovable.js'; +import Bounds2 from '../../../../dot/js/Bounds2.js'; // constants const BACKGROUND_ENABLED_FILL = 'rgb( 31, 113, 2 )'; const BACKGROUND_DISABLED_FILL = 'rgb( 178, 178, 178 )'; +const BACKGROUND_X_MARGIN = 14; +const BACKGROUND_Y_MARGIN = 10; +const BACKGROUND_Y_SPACING = 6; +const BACKGROUND_CORNER_RADIUS = 12; +const NON_ZERO_BOUNDS = new Bounds2( 0, 0, 1, 1 ); +const HIGHLIGHT_LINE_WIDTH = 3; const SCALE_SIZE = new Dimension2( 55, 450 ); const SCALE_LABEL_FONT = new PhetFont( { size: 30, weight: 'bold' } ); const TICK_LENGTH = 15; const TICK_FONT = new PhetFont( 22 ); const NEUTRAL_TICK_LENGTH = 40; const TICK_LABEL_X_SPACING = 5; -const CORNER_RADIUS = 12; type SelfOptions = EmptySelfOptions; @@ -352,19 +358,19 @@ const options = optionize()( {}, providedOptions ); - // dashed line that extends across the scale - const lineNode = new Line( 0, 0, scaleWidth, 0, { - stroke: 'black', - lineDash: [ 5, 5 ], - lineWidth: 2 + // 'pH' label above the value + const pHText = new Text( PhScaleStrings.pHStringProperty, { + fill: 'white', + font: new PhetFont( { size: 28, weight: 'bold' } ), + maxWidth: 100 } ); - // pH display + // pH value display const numberDisplay = new NumberDisplay( pHProperty, PHScaleConstants.PH_RANGE, { decimalPlaces: PHScaleConstants.PH_METER_DECIMAL_PLACES, align: 'right', noValueAlign: 'center', - cornerRadius: CORNER_RADIUS, + cornerRadius: 6, xMargin: 8, yMargin: 5, textOptions: { @@ -374,36 +380,29 @@ tandem: options.tandem.createTandem( 'numberDisplay' ) } ); - // label above the value - const pHText = new Text( PhScaleStrings.pHStringProperty, { - fill: 'white', - font: new PhetFont( { size: 28, weight: 'bold' } ), - maxWidth: 100 + const content = new VBox( { + children: [ pHText, numberDisplay ], + spacing: BACKGROUND_Y_SPACING, + align: 'center' } ); - // background - const backgroundXMargin = 14; - const backgroundYMargin = 10; - const backgroundYSpacing = 6; - const backgroundWidth = Math.max( pHText.width, numberDisplay.width ) + ( 2 * backgroundXMargin ); - const backgroundHeight = pHText.height + numberDisplay.height + backgroundYSpacing + ( 2 * backgroundYMargin ); - const backgroundRectangle = new Rectangle( 0, 0, backgroundWidth, backgroundHeight, { - cornerRadius: CORNER_RADIUS, - fill: BACKGROUND_ENABLED_FILL + // background, correct size set below + const backgroundRectangle = new Rectangle( 0, 0, 1, 1, { + fill: BACKGROUND_ENABLED_FILL, + cornerRadius: BACKGROUND_CORNER_RADIUS } ); - // highlight around the background - const highlightLineWidth = 3; - const outerHighlight = new Rectangle( 0, 0, backgroundWidth, backgroundHeight, { - cornerRadius: CORNER_RADIUS, + // highlight around the background, correct size set below + const outerHighlight = new Rectangle( 0, 0, 1, 1, { stroke: 'black', - lineWidth: highlightLineWidth + cornerRadius: BACKGROUND_CORNER_RADIUS, + lineWidth: HIGHLIGHT_LINE_WIDTH } ); - const innerHighlight = new Rectangle( highlightLineWidth, highlightLineWidth, - backgroundWidth - ( 2 * highlightLineWidth ), backgroundHeight - ( 2 * highlightLineWidth ), { - cornerRadius: CORNER_RADIUS, - stroke: 'white', lineWidth: highlightLineWidth - } ); + const innerHighlight = new Rectangle( 0, 0, 1, 1, { + stroke: 'white', + cornerRadius: BACKGROUND_CORNER_RADIUS, + lineWidth: HIGHLIGHT_LINE_WIDTH + } ); const highlight = new Node( { children: [ innerHighlight, outerHighlight ], visible: false @@ -418,34 +417,49 @@ .close(); const arrowNode = new Path( arrowShape, { fill: 'black' } ); - // layout, origin at arrow tip + // dashed line that extends across the scale + const lineNode = new Line( 0, 0, scaleWidth, 0, { + stroke: 'black', + lineDash: [ 5, 5 ], + lineWidth: 2 + } ); + + // layout, origin at left end of lineNode lineNode.left = 0; lineNode.centerY = 0; arrowNode.left = lineNode.right; arrowNode.centerY = lineNode.centerY; - backgroundRectangle.left = arrowNode.right - 1; // overlap to hide seam - backgroundRectangle.centerY = arrowNode.centerY; - highlight.center = backgroundRectangle.center; + + // If parts of the content are made invisible via PhET-iO, resize the background. + // See https://github.com/phetsims/ph-scale/issues/272 + content.boundsProperty.link( bounds => { + bounds = bounds.equals( Bounds2.NOTHING ) ? NON_ZERO_BOUNDS : bounds; + + // Resize the background + backgroundRectangle.setRectBounds( bounds.dilatedXY( BACKGROUND_X_MARGIN, BACKGROUND_Y_MARGIN ) ); + backgroundRectangle.left = arrowNode.right - 1; // overlap to hide seam + backgroundRectangle.centerY = arrowNode.centerY; + + // Center content in the background + content.center = backgroundRectangle.center; + + // Resize the highlight + outerHighlight.setRectBounds( backgroundRectangle.bounds ); + outerHighlight.center = backgroundRectangle.center; + innerHighlight.setRectBounds( backgroundRectangle.bounds.eroded( HIGHLIGHT_LINE_WIDTH ) ); + innerHighlight.center = backgroundRectangle.center; + } ); options.children = [ arrowNode, backgroundRectangle, highlight, - pHText, - numberDisplay, + content, lineNode ]; super( options ); - pHText.boundsProperty.link( bounds => { - pHText.centerX = backgroundRectangle.centerX; - pHText.top = backgroundRectangle.top + backgroundYMargin; - } ); - - numberDisplay.centerX = backgroundRectangle.centerX; - numberDisplay.top = pHText.bottom + backgroundYSpacing; - pHProperty.link( pH => { // make the indicator look enabled or disabled ```

[pixelzoom]

My questions:

• I'm doubting the validity of this change request. Is the pH indicator useful when the NumberDisplay is hidden? Is the highlighting of the indicator still meaningful when no value is shown? Does the indicator even look OK when it resizes around the tiny 'pH' label? (It looks ludicrous to me.)

• I'm pushing back on the timing of this change request. Has anyone actually asked for this, or is this hypothetical? And is this the kind of change that we should be making during an RC?

• Would it be better to uninstrument pHMeterNode.pHIndicatorNode.numberDisplay.visibleProperty, and wait until someone asks for this?

• If we were going to design a pH meter whose NumberDisplay could be hidden, would it look like this? I doubt it.

• The pH indicator becomes highlighted (gets a black and white stroked border) when pH=7. And this method of highlighting is what makes this component complicated to resize. Is this highlight useful? Does anyone even notice? Is it still useful if the NumberDisplay is hidden? Could we do something different to emphasize "neutral pH", like change the color of the NumberDisplay background?

[arouinfar]

@pixelzoom thanks for looking into this. After reviewing your comments, I don't think it's something we need to do. I thought the lack of dynamic layout was an oversight.

I'm doubting the validity of this change request. Is the pH indicator useful when the NumberDisplay is hidden? Is the highlighting of the indicator still meaningful when no value is shown? Does the indicator even look OK when it resizes around the tiny 'pH' label? (It looks ludicrous to me.)

I think the original idea is that clients may want students to read the value from the scale rather than the exact value from the display to support measurement-oriented goals. I honestly forgot about the highlight that is displayed when the pH = 7 (more on that later). If the indicator simply got shorter, it would probably look fine. I'm guessing it gets narrower with dynamic layout, and I can see how that would look ridiculous.

I'm pushing back on the timing of this change request. Has anyone actually asked for this, or is this hypothetical? And is this the kind of change that we should be making during an RC?

Hypothetical, but it seemed like the issue was an oversight with dynamic layout. I didn't realize it wasn't a standard panel. In this case, no, it's not a change we need to make during RC.

Would it be better to uninstrument pHMeterNode.pHIndicatorNode.numberDisplay.visibleProperty, and wait until someone asks for this?

Let's leave it as-is.

If we were going to design a pH meter whose NumberDisplay could be hidden, would it look like this? I doubt it.

Probably not, no.

The pH indicator becomes highlighted (gets a black and white stroked border) when pH=7. And this method of highlighting is what makes this component complicated to resize. Is this highlight useful? Does anyone even notice? Is it still useful if the NumberDisplay is hidden? Could we do something different to emphasize "neutral pH", like change the color of the NumberDisplay background?

I think the "neutral" label that appears in the beaker when the pH is exactly 7 is far more noticeable than the highlight on the pH indicator. The highlight is very subtle, and it's something I've overlooked.

[pixelzoom]

👍🏻 Thanks @arouinfar.