search

phetsims / circuit-construction-kit-common

"Circuit Construction Kit: Basics" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
10 stars 10 forks source link

Should labels in carousel be instrumented? #869

Closed arouinfar closed 1 year ago

arouinfar commented 2 years ago

The labels in the toolbox are instrumented and clients can customize the strings. image image

Should we do the same for the labels in the carousel?

image
arouinfar commented 2 years ago

2/24/22 design meeting, we would like to make the strings in the carousel customizable. Additionally, the visibleProperty should be read-only because the labels are controlled by the Labels checkbox.

samreid commented 2 years ago

Patch with current status:

```diff Index: js/view/CircuitElementToolNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/view/CircuitElementToolNode.ts b/js/view/CircuitElementToolNode.ts --- a/js/view/CircuitElementToolNode.ts (revision e04cc3b4724ea7486dfbe46ad27e4edb6b01e192) +++ b/js/view/CircuitElementToolNode.ts (date 1646872159589) @@ -45,14 +45,21 @@ constructor( labelText: string, showLabelsProperty: Property, viewTypeProperty: Property, circuit: Circuit, globalToCircuitLayerNodePoint: ( v: Vector2 ) => Vector2, iconNode: Node, maxNumber: number, count: () => number, createElement: ( v: Vector2 ) => CircuitElement, providedOptions?: any ) { - const labelNode = new Text( labelText, { fontSize: 12, maxWidth: TOOLBOX_ICON_WIDTH } ); - showLabelsProperty.linkAttribute( labelNode, 'visible' ); + + let labelNode: Node | any = null; + if ( labelText.length > 0 ) { + labelNode = new Text( labelText, { + fontSize: 12, maxWidth: TOOLBOX_ICON_WIDTH, + tandem: providedOptions.tandem.createTandem( 'label' ) + } ); + showLabelsProperty.linkAttribute( labelNode, 'visible' ); + } providedOptions = merge( { spacing: 2, // Spacing between the icon and the text cursor: 'pointer', // hack because the series ammeter tool node has text rendered separately (joined with probe ammeter) - children: labelText.length > 0 ? [ iconNode, labelNode ] : [ iconNode ], + children: labelNode ? [ iconNode, labelNode ] : [ iconNode ], // Expand touch area around text, see https://github.com/phetsims/circuit-construction-kit-dc/issues/82 touchAreaExpansionLeft: 0, ```
samreid commented 2 years ago

Implemented above (and made visibleProperty readonly). Ready for review.

arouinfar commented 1 year ago

Thanks @samreid. I'm not sure where these changes fell in relation to the changes made in https://github.com/phetsims/chipper/issues/1302. However, reviewing master, each carousel item has a link back to the appropriate stringProperty in general.model.strings, which is what I would expect to see. Closing.