PhET-iO design review comments

[arouinfar]

@kathy-phet and I reviewed this sim in studio, and our comments are below. @jonathanolson please let me know if you have any questions or would like to have a zoom meeting to discuss. Please feel free to push back on any change that you feel is overly invasive or inappropriate.

• [x] Values should include units, where applicable.

  • [x] model.amplitudeProperty: cm
  • [x] model.angleProperty: rad
  • [x] model.frequencyProperty: Hz
  • [x] model.pulseWidthProperty: s

• [x] Several properties provide the location of a UI element in view coordinates. The location of the origin should be documented as well as the reference point for each item (e.g. center, upper-left corner).

  • [x] model.horizontalRulerPositionProperty
  • [x] model.verticalRulerPositionProperty
  • [x] model.referenceLinePositionProperty
  • [x] model.stopwatch.positionProperty

• [x] The model has lots of properties that look like implementation details. Confirm that these are all necessary to instrument. If necessary for restoring state, no action is necessary.

• [x] model.timeControlSpeedProperty remove "s" from "times" in documentation (should be singular).

• [x] Create a new model property called waveStartPositionProperty which is the y-value of the 1st green dot measured with respect to the centerLine in cm.

• [x] The stopwatch has 3 visibleProperties associated with it which can get confusing. Setting model.stopwatchVisibleProperty to true will make the stopwatch visible and check its checkbox. model.stopwatch.isVisibleProperty and view.stopwatchNode.visibleProperty will make the stopwatch visible, but the checkbox is unchecked. Would it be possible have only model.stopwatch.isVisibleProperty and hook it up to the checkbox? view.bottomControlPanel.visibilityCheckboxGroup.stopwatchVisibleCheckbox would contain a linked property to model.stopwatch.isVisibleProperty. view.stopwatchNode already has a link to model.stopwatch which is sufficient.

• [x] Update view.startNode.wrenchDragAndDropHandler to modern dragListener and call it wrenchDragListener, if possible.

• [x] Make these phetioReadOnly: true

  • [x] view.endNode.opacityProperty
  • [x] view.endNode.pickableProperty
  • [x] view.endNode.visibleProperty
  • [x] view.startNode.opacityProperty
  • [x] view.startNode.pickableProperty
  • [x] view.startNode.visibleProperty
  • [x] view.stringNode.opacityProperty
  • [x] view.stringNode.pickableProperty
  • [x] view.stringNode.visibleProperty

• [x] The view could use some organization to make things easier to find in the tree. Create a container called wavePlayArea.

  -wavePlayArea
  +centerLine
  +endNode
  +startNode
  +stringNode

• [x] Similarly, it would be nice to group the rulers together in the tree under something like rulersNode.

  -rulersNode
  +horizontalRulerNode
  +verticalRulerNode

• [x] As discussed in the design meeting, it would be nice to allow clients to only use one of the rulers, and hide the other one. Currently each ruler has a visibleProperty, but that will get reset every time the Rulers checkbox is toggled. We faced a similar challenge with phScale.macroScreen.view.neutralIndicatorNode in https://github.com/phetsims/ph-scale/issues/102.

• [x] We would like to add a top-level visibleProperty to view.bottomControlPanel so clients can hide the entire panel by toggling one property. (If opacityProperty & pickableProperty come along for the ride that's okay. Not sure how soon those are going to be eliminated based on today's discussion in PhET-iO meeting, see https://github.com/phetsims/scenery/issues/1047).

• [x] Related to the above item, each sub-panel (manualPanel, oscillatePanel, pulsePanel) within the bottomControlPanel would need to listen to view.bottomControlPanel.visibleProperty. However, if these sub-panels can be uninstrumented entirely, that would be preferable.

• [x] It would be nice, but not necessary, if the bottomControlPanel could leverage dynamic layout and resize if controls are removed.

• [x] Several tandem names could be clearer. We recommend renaming the following:

  • [x] model.modeProperty > model.waveModeProperty
  • [x] model.timeProperty > model.timeElapsedProperty
  • [x] view.bottomControlPanel > view.controlPanel
  • [x] view.bottomControlPanel.visibilityCheckboxGroup > view.controlPanel.checkboxGroup
  • [x] view.bottomControlPanel.visibilityCheckboxGroup.referenceLineVisibleCheckbox > view.controlPanel.checkboxGroup.referenceLineCheckbox
  • [x] view.bottomControlPanel.visibilityCheckboxGroup.rulersVisibleCheckbox > view.controlPanel.checkboxGroup.rulersCheckbox
  • [x] view.bottomControlPanel.visibilityCheckboxGroup.stopwatchVisibleCheckbox > view.controlPanel.checkboxGroup.stopwatchCheckbox
  • [x] view.endTypePanel.endTypeRadioGroup > view.endTypePanel.radioButtonGroup
  • [x] view.endTypePanel.endTypeRadioGroup.fixedEndButton > view.endTypePanel.radioButtonGroup.fixedEnd
  • [x] view.endTypePanel.endTypeRadioGroup.looseEndButton > view.endTypePanel.radioButtonGroup.looseEnd
  • [x] view.endTypePanel.endTypeRadioGroup.noEndButton > view.endTypePanel.radioButtonGroup.noEnd
  • [x] view.modePanel > view.waveModePanel
  • [x] view.modePanel.modeRadioGroup > view.waveModePanel.radioButtonGroup
  • [x] view.modePanel.modeRadioGroup.manualButton > view.waveModePanel.radioButtonGroup.manual
  • [x] view.modePanel.modeRadioGroup.oscillateButton > view.waveModePanel.radioButtonGroup.oscillate
  • [x] view.modePanel.modeRadioGroup.pulseButton > view.waveModePanel.radioButtonGroup.pulse

[jonathanolson]

I believe I've implemented the above changes on master, can you verify and see how things look?

[arouinfar]

Unassigning myself until work resumes on this sim.