Preliminary review of view code.

[pixelzoom]

Similar to the model review that I did in https://github.com/phetsims/number-pairs/issues/9#issuecomment-2375275553 ...

Most of the view code is in number-pairs/common/view/.

Focus on these classes first:

• CubesOnWireNode
• NumberLineNode
• NumberLineSlider

[pixelzoom]

@marlitas The view is looking really great - some nice solutions to complicated problems. Well done!

Below is a check list of things thast I noted. I'm going to add to this list as I go, and will assign to you to review when I'm done. Let me know if you'd like to discuss any of these.

---

• [x] CubesOnWireNode: Say something in the header doc about the implications of the cubes being on a wire. Because they are on a wire, dragging a cube left/right also moves the other cubes to its left/right. (You may recall that this is a distinction that I didn’t pick up on immediately. :)

---

• [x] CubesOnWireNode: Duplicate code for creating wireMinXEndCap and wireMaxXEndCap. Create inner class EndCap extends Path.

---

• [x] CubesOnWireNode: snapCubesToPositions and handleCubeMove should be private.

---

• [x] CubesOnWireNode: calculateCubeSeparatorPlacement does not need to be a class method. It could be a function outside the class definition.

---

• [x] NumberLineNode: superOptions is misleading because it’s not just the options for the superclass, it also includes SelfOptions. It’s also not the convention for the PhET Options pattern. Rename to options.

---

• [x] NumberLineNode: As I understand it zeroNumberProperty should never change. This would be a bit safer:

// EllipticalArrowNode needs the starting and ending values to be Property<number> instances. Even though
// the starting value for the leftAddendArrow and totalArrow will always be 0.
- const zeroNumberProperty = new Property( options.numberLineRange.min );
+ const zeroNumberProperty = new Property( options.numberLineRange.min, {
+   isValidValue: value => value === options.numberLineRange.min
+ } );

---

• [x] There are many misuses of combineOptions. For example, see CubeNode. The type of providedOptions is CubeNodeOptions, and optionize should be used instead of combineOptions.

- const options = combineOptions<NodeOptions>( {
+ const options = optionize<CubeNodeOptions, SelfOptions, NodeOptions>()( {
  children: [ cubeLeftAddendImage, cubeRightAddendImage ],
  visibleProperty: new DerivedProperty( [ model.addendTypeProperty ], addendType => addendType !== AddendType.INACTIVE )
}, providedOptions );

There are similar misuses of combineOptions in:

• CubesOnWireNode:
• EllipticalArrowNode:
• KittenNode
• KittenLayerNode:
• LocationCountingObjectNode:
• NumberBondNode
• NumberCircle
• NumberLineNode
• NumberLineOptionsCheckboxGroup
• NumberLineSlider

---

• [x] NumberLineOptionsCheckboxGroup: options does not conform to the PhET options pattern. Define NumberLineOptionsCheckboxGroupOptions and use optionize. You may also wish to exclude NodeTranslationOptions if this class is really responsive for translation.

public constructor( model: NumberPairsModel, countingAreaBounds: Bounds2, providedOptions: WithRequired<VerticalCheckboxGroupOptions, 'tandem'> ) {
  const options = combineOptions<VerticalCheckboxGroupOptions>( {
    bottom: countingAreaBounds.top - 8,
    right: countingAreaBounds.right
  }, providedOptions );

---

• [x] SceneSelectionRadioButtonGroupOptions is a stale type name, because the associated class was renamed to TotalRadioButtonGroup:

- type SceneSelectionRadioButtonGroupOptions = SelfOptions & RectangularRadioButtonGroupOptions;
+ type TotalRadioButtonGroupOptions = SelfOptions & RectangularRadioButtonGroupOptions;

---

• [x] SpeechSynthesisButton: providedOptions should be required because SpeechSynthesisButtonOptions includes required options.

- public constructor( providedOptions?: SpeechSynthesisButtonOptions )
+ public constructor( providedOptions: SpeechSynthesisButtonOptions )

---

• [x] SplitCountingAreaNode: the type of provideOptions violates the PhET options pattern. And I can break it via providedOptions.children.

- public constructor( countingAreaBounds: Bounds2, providedOptions?: NodeOptions ) {
+ type SelfOptions = EmptySelfOptions
+ type SplitCountingAreaNode = SelfOptions & StrictOmit<NodeOptions, ‘children’>;
+ public constructor( countingAreaBounds: Bounds2, providedOptions?: SplitCountingAreaNodeOptions ) {

---

• [x] Valid values for countingRepresentationTypeProperty should be obtained from that Property.

In NumberPairsModel, countingRepresentationTypeProperty should probably have validValues, instead of relying on the default for EnumerationProperty:

this.countingRepresentationTypeProperty = new EnumerationProperty( options.initialCountingRepresentationType, {
  tandem: options.tandem.createTandem( 'countingRepresentationTypeProperty' )
} );

Where countingRepresentationTypeProperty is used in the view, it would be preferrable to get the validValues from countingRepresentationTypeProperty, rather than as a separate arg or options. Otherwise, you run the risk of providing an invalid set of values.

For example, in CountingRepresentationRadioButtonGroup:

- const groupItems = providedOptions.countingRepresentations.map( countingRepresentationType => {
+ const groupItems = countingRepresentationTypeProperty.validValues.map( countingRepresentationType => {

---

• [x] Consider dropping “Number” in the names of NumberProperty instances where it is somewhat redundant. Shorter names, and more typical for PhET-iO. For example:

leftAddendNumberProperty => leftAddendProperty rightAddendNumberProperty => rightAddendProperty totalNumberProperty => totalProperty

---

• [x] Consider dropping the work “Model” from the names of model classes, Properties, etc. PhET typically names things like Ball (model) and BallNode (view), not BallModel. A “Model” suffix is only used to clarify confusing cases. Shorter names, and more typical for PhET-iO. For example:

- public constructor( selectedSceneModelProperty: PhetioProperty<NumberPairsSceneModel>, sceneModels: NumberPairsSceneModel[],
+ public constructor( selectedSceneProperty: PhetioProperty<NumberPairsScene>, scenes: NumberPairsScene[],

---

• [x] Compose SelfOptions before parent options, to correspond to the PhET options pattern. For example:

- type NumberLineSliderOptions = WithRequired<SliderOptions, 'tandem'> & SelfOptions;
+ type NumberLineSliderOptions = SelfOptions & WithRequired<SliderOptions, 'tandem'>;

---

• [x] Tighten up Option type definitions. Omit options that are the responsibility of the class and would break the implementation.

In my experience, sim code is more robust if you add only the options that are actually needed, so that options are not set unintentionally, and so that you don’t need to be vigilant about revisiting options when you add responsibilities to a class. I realize that not everyone shares that philosophy, so up to you about how "narrow" to make your Option types. But they should not be so "wide" that options that can break the implementation are exposed.

Here are examples, with a comment indicating which options I can set to break the implementation.

// content, listener
type CommutativeButtonOptions = SelfOptions & WithRequired<RectangularPushButtonOptions, 'tandem'>;

// content, listener
type TenFrameButtonOptions = SelfOptions & WithRequired<RectangularPushButtonOptions, 'tandem'>;

// content
type SpeechSynthesisButtonOptions = WithRequired<RectangularPushButtonOptions, 'tandem'>;

// visibilityProperty
type CubeNodeOptions = SelfOptions & PickRequired<NodeOptions, 'tandem'> & StrictOmit<NodeOptions, 'children'>;

// radius, because it’s also passed in as the first constructor arg, and CircleOptions.radius will override that.
type NumberCircleOptions = StrictOmit<CircleOptions, 'children'>;

// rectSize, which is computed from the squareDimension constructor arg.
type NumberSquareOptions = SelfOptions & StrictOmit<RectangleOptions, 'children'>;

// thumbNode, trackNode, constrainValue, enabledRangeProperty
type NumberLineSliderOptions = WithRequired<SliderOptions, 'tandem'> & SelfOptions;

// orientation
type CountingRepresentationRadioButtonGroupOptions = SelfOptions & WithRequired<RectangularRadioButtonGroupOptions, 'tandem'>;

// children
export type NumberPairsScreenViewOptions = SelfOptions & WithRequired<ScreenViewOptions, 'tandem'>;

// Is being able to override showTitleWhenExpanded desirable?
export type TotalRepresentationAccordionBoxOptions = WithRequired<AccordionBoxOptions, 'titleNode' | 'tandem'>;

[pixelzoom]

Review completed, over to you @marlitas.

[marlitas]

You may also wish to exclude NodeTranslationOptions if this class is really responsive for translation.

I would like those translation options to be the default, but I don't think passing in a different translation in a non-default use case would break the component.

I have a question about options pattern that I documented with this issue number in KittenNode

Valid values for countingRepresentationTypeProperty should be obtained from that Property.

This is an option because each screen has a different set of counting representation types. I can't determine that based on valid values. Perhaps an assertion to confirm that any values I pass in are of CountingRepresentationType. But I thought that would be covered by type checking with the option's type defined as: CountingRepresentationType[]. Or wait... perhaps you're indicating that the valid values for that Property are specifically defined for each screen? Let's confirm I'm understanding on Monday.

Is being able to override showTitleWhenExpanded desirable?

Mmmmm... maybe? I don't think it breaks anything and it feels like an unnecessary way to tie hands in the future if we do want it for one of the accordion boxes.

[pixelzoom]

Or wait... perhaps you're indicating that the valid values for that Property are specifically defined for each screen?

Correct. The default for validValues in this case is CountingRepresentationType.enumeration.values. That is not appropriate for all screens -- validValues should be the subset of CountingRepresentationType.enumeration.values supported by the screen. Otherwise you (or a PhET-iO client!) can set countingRepresentationTypeProperty to a value that is not supported by a screen.

[marlitas]

I believe all of the items in this issue have been addressed. @pixelzoom feel free to re-open if you disagree.