Address review comments, see: #78

js/common/MeanShareAndBalanceConstants.ts

@@ -26,16 +26,13 @@ const MeanShareAndBalanceConstants = {
   CONTROLS_VERTICAL_MARGIN: 30,
   CONTROLS_HORIZONTAL_MARGIN: 15,
 
-  // Sets center Y value for cups.
-  //REVIEW If these set centerY, then they should be named CUPS_3D_CENTER_Y and CUPS_2D_CENTER_Y. The current names correspond to a Node's y property.
-  CUPS_3D_Y_VALUE: 525,
-  CUPS_2D_Y_VALUE: 275,
+  CUPS_3D_CENTER_Y: 525,
+  CUPS_2D_CENTER_Y: 275,
 
   INITIAL_NUMBER_OF_PEOPLE: 1,
 
-  //REVIEW These names are a little vague/incorrect. MOUSE_AREA_DILATION and TOUCH_AREA_DILATION dilation would be clearer.
-  MOUSE_DILATION: 5,
-  TOUCH_DILATION: 10,
+  MOUSE_AREA_DILATION: 5,
+  TOUCH_AREA_DILATION: 10,
 
   MAX_CONTROLS_TEXT_WIDTH: 175
 

js/common/view/MeanShareAndBalanceScreenView.ts

@@ -1,8 +1,8 @@
 // Copyright 2022, University of Colorado Boulder
 
-//REVIEW This is an incomplete description of MeanShareAndBalanceScreen, and resetAll isn't really significant.
 /**
- * Representation for the MeanShareAndBalanceScreen, contains resetAll button
+ * Parent ScreenView that contains components shared across screens such as: QuestionBar, Controls Layout,
+ * SyncDataButton, and ResetAll
  *
  * @author Marla Schulz (PhET Interactive Simulations)
  * @author Sam Reid (PhET Interactive Simulations)
@@ -29,16 +29,15 @@ export type MeanShareAndBalanceScreenViewOptions = SelfOptions & PickRequired<Sc
 
 export default class MeanShareAndBalanceScreenView extends ScreenView {
   public readonly resetAllButton: ResetAllButton;
-  //REVIEW more descriptive name or doc. What "data" is this synchronizing?
-  public readonly syncDataButton: RectangularPushButton;
+  // Syncs the two representations on each screen. ie. in intro, syncs the 3D and 2D cups.
+  public readonly syncRepresentationsButton: RectangularPushButton;
   public readonly controlsVBox: VBox;
   public readonly numberSpinnerVBox: VBox;
   public readonly questionBar: QuestionBar;
 
   private readonly controlsAlignBox: AlignBox;
 
-  //REVIEW If you don't intend MeanShareAndBalanceScreenView to be instantiated directly, then constructor should be protected.
-  public constructor( model: MeanShareAndBalanceModel, providedOptions: MeanShareAndBalanceScreenViewOptions ) {
+  protected constructor( model: MeanShareAndBalanceModel, providedOptions: MeanShareAndBalanceScreenViewOptions ) {
     const options = optionize<MeanShareAndBalanceScreenViewOptions, SelfOptions, ScreenViewOptions>()( {}, providedOptions );
 
     super( options );
@@ -68,7 +67,7 @@ export default class MeanShareAndBalanceScreenView extends ScreenView {
       ]
     } );
 
-    this.syncDataButton = new RectangularPushButton( {
+    this.syncRepresentationsButton = new RectangularPushButton( {
       listener: () => {
         this.interruptSubtreeInput(); // cancel interactions that may be in progress
         model.syncData();
@@ -77,7 +76,6 @@ export default class MeanShareAndBalanceScreenView extends ScreenView {
       accessibleName: meanShareAndBalanceStrings.sync,
       right: this.layoutBounds.maxX - MeanShareAndBalanceConstants.CONTROLS_HORIZONTAL_MARGIN,
       baseColor: 'white',
-      //REVIEW tandem name does not match this.syncDataButton
       tandem: options.tandem.createTandem( 'syncRepresentationsButton' ),
       layoutOptions: { column: 1, row: 1, xAlign: 'left', minContentHeight: 140, yAlign: 'top' }
     } );
@@ -108,7 +106,7 @@ export default class MeanShareAndBalanceScreenView extends ScreenView {
     const controlsGridBox = new GridBox( {
       children: [
         this.controlsVBox,
-        this.syncDataButton,
+        this.syncRepresentationsButton,
         this.numberSpinnerVBox
       ],
       minContentWidth: MeanShareAndBalanceConstants.MAX_CONTROLS_TEXT_WIDTH + 25,
@@ -128,23 +126,12 @@ export default class MeanShareAndBalanceScreenView extends ScreenView {
     this.addChild( this.controlsAlignBox );
   }
 
-  //REVIEW Since this does nothing, I would delete it until it's actually needed by a future screen.
   /**
    * Resets the view.
    */
   public reset(): void {
     // May be used for future screens
   }
-
-  //REVIEW This is not necessary. ScreenView already has an identical (do nothing) implementation. Add this if/when ScreenView does something different.
-  //REVIEW It's also dangerous to override without calling super.step().  If ScreenView implementation actually does something in the future, it won't get done.
-  /**
-   * Steps the view.
-   * @param dt - time step, in seconds
-   */
-  public override step( dt: number ): void {
-    // See subclass for implementation
-  }
 }
 
 meanShareAndBalance.register( 'MeanShareAndBalanceScreenView', MeanShareAndBalanceScreenView );

js/intro/model/IntroModel.ts

@@ -1,8 +1,7 @@
 // Copyright 2022, University of Colorado Boulder
 
-//REVIEW what is "Leveling Out Screen"? Do you mean "Intro screen"?
 /**
- * Model for the Leveling Out screen, which includes 2D cups, 3D cups, connecting pipes, and view options.
+ * Model for the Intro screen, which includes 2D cups, 3D cups, connecting pipes, and view options.
  *
  * @author Marla Schulz (PhET Interactive Simulations)
  * @author Sam Reid (PhET Interactive Simulations)
@@ -37,7 +36,7 @@ export default class IntroModel extends MeanShareAndBalanceModel {
   public readonly isShowingPredictMeanProperty: BooleanProperty;
   public readonly isShowingMeanProperty: BooleanProperty;
   public readonly isShowingTickMarksProperty: BooleanProperty;
-  //REVIEW Document isAutoSharingProperty. I have no idea what this does, never figure it out during code review.
+  // Property that tracks whether auto-share is enabled or not.
   public readonly isAutoSharingProperty: BooleanProperty;
   public readonly numberOfCupsProperty: NumberProperty;
   public readonly meanPredictionProperty: NumberProperty;
@@ -83,7 +82,7 @@ export default class IntroModel extends MeanShareAndBalanceModel {
       return new WaterCupModel( {
         tandem: tandem,
         x: x,
-        y: MeanShareAndBalanceConstants.CUPS_3D_Y_VALUE
+        y: MeanShareAndBalanceConstants.CUPS_3D_CENTER_Y
       } );
     }, [ 0 ], {
       phetioType: PhetioGroup.PhetioGroupIO( WaterCupModel.WaterCupModelIO ),
@@ -95,7 +94,7 @@ export default class IntroModel extends MeanShareAndBalanceModel {
       return new WaterCupModel( {
         tandem: tandem,
         x: x,
-        y: MeanShareAndBalanceConstants.CUPS_2D_Y_VALUE,
+        y: MeanShareAndBalanceConstants.CUPS_2D_CENTER_Y,
         waterLevelPropertyOptions: {
           phetioReadOnly: true
         }

js/intro/model/PipeModel.ts

@@ -1,9 +1,10 @@
 // Copyright 2022, University of Colorado Boulder
 
-//REVIEW Incorrect doc. This is not a base class for PipeNode. It's the model for the pipe.
-//REVIEW Needs more description - what is a "pipe", etc.
 /**
- * Base class for Pipe Node
+ * Model for pipes and valves.
+ * The pipes visually connect the 2D cup representations and the valves control whether water is shared or not.
+ * When the valves are open the water flows between all connected cups, and when the valves are closed
+ * water is not allowed to flow between cups.
  *
  * @author Marla Schulz (PhET Interactive Simulations)
  * @author Sam Reid (PhET Interactive Simulations)
@@ -23,9 +24,8 @@ type StateObject = {
 }
 
 type SelfOptions = {
-  //REVIEW non-obvious options are supposed to be documented where defined, and these are not obvious to the reviewer
-  x: number;
-  y: number;
+  x: number; // the x-position of the pipe in the view
+  y: number; // the y-position of the pipe in the view
   isOpen?: boolean;
 };
 
@@ -34,9 +34,11 @@ export type PipeModelOptions = SelfOptions & PhetioObjectOptions;
 
 export default class PipeModel extends PhetioObject {
 
-  //REVIEW non-obvious fields are supposed to be documented where declared, and these are not obvious to the reviewer
+  // Property tracks whether pipe's valve is open or not.
   public readonly isOpenProperty: BooleanProperty;
+  // Property tracks if the pipe's valve is in a clicked state.
   public readonly isCurrentlyClickedProperty = new BooleanProperty( false );
+  // The x and y positions of the pipe in the view.
   public readonly x: number;
   public readonly y: number;
 

js/intro/view/IntroScreenView.ts

@@ -1,8 +1,6 @@
 // Copyright 2022, University of Colorado Boulder
-
-//REVIEW what is "Leveling Out Screen"? Do you mean "Intro screen"?
 /**
- * Representation for the Leveling Out Screen, displaying 2D/3D water cups, pipes, and various interactive options.
+ * Representation for the Intro Screen, displaying 2D/3D water cups, pipes, and various interactive options.
  *
  * @author Marla Schulz (PhET Interactive Simulations)
  * @author Sam Reid (PhET Interactive Simulations)
@@ -45,8 +43,8 @@ export default class IntroScreenView extends MeanShareAndBalanceScreenView {
 
     super( model, options );
 
-    const modelViewTransform2DCups = ModelViewTransform2.createSinglePointScaleInvertedYMapping( new Vector2( 0, 0 ), new Vector2( 0, MeanShareAndBalanceConstants.CUPS_2D_Y_VALUE ), MeanShareAndBalanceConstants.CUP_HEIGHT );
-    const modelViewTransform3DCups = ModelViewTransform2.createSinglePointScaleInvertedYMapping( new Vector2( 0, 0 ), new Vector2( 0, MeanShareAndBalanceConstants.CUPS_3D_Y_VALUE ), MeanShareAndBalanceConstants.CUP_HEIGHT );
+    const modelViewTransform2DCups = ModelViewTransform2.createSinglePointScaleInvertedYMapping( new Vector2( 0, 0 ), new Vector2( 0, MeanShareAndBalanceConstants.CUPS_2D_CENTER_Y ), MeanShareAndBalanceConstants.CUP_HEIGHT );
+    const modelViewTransform3DCups = ModelViewTransform2.createSinglePointScaleInvertedYMapping( new Vector2( 0, 0 ), new Vector2( 0, MeanShareAndBalanceConstants.CUPS_3D_CENTER_Y ), MeanShareAndBalanceConstants.CUP_HEIGHT );
 
     const predictMeanText = new Text( meanShareAndBalanceStrings.predictMean, { fontSize: 15, maxWidth: MeanShareAndBalanceConstants.MAX_CONTROLS_TEXT_WIDTH } );
     const showMeanText = new Text( meanShareAndBalanceStrings.showMean, { fontSize: 15, maxWidth: MeanShareAndBalanceConstants.MAX_CONTROLS_TEXT_WIDTH } );
@@ -224,16 +222,16 @@ export default class IntroScreenView extends MeanShareAndBalanceScreenView {
       predictMeanLine,
       numberOfCupsNumberSpinner,
       waterCupLayerNode,
-      this.syncDataButton
+      this.syncRepresentationsButton
     ];
 
     this.pdomControlAreaNode.pdomOrder = [
       this.resetAllButton
     ];
   }
 
-  //REVIEW Missing call to super.step()? If super.step actually does something in the future, it won't get done.
   public override step( dt: number ): void {
+    super.step( dt );
     for ( const pipe of this.pipeMap.values() ) {
       pipe.step( dt );
     }

js/intro/view/PipeNode.ts

@@ -133,8 +133,8 @@ export default class PipeNode extends Node {
     this.pipeRectangle.clipArea = createPipeClipArea( this.pipeRectangle.localBounds, VALVE_RADIUS );
 
     // Set pointer areas for valveNode
-    this.valveNode.mouseArea = this.valveNode.localBounds.dilated( MeanShareAndBalanceConstants.MOUSE_DILATION );
-    this.valveNode.touchArea = this.valveNode.localBounds.dilated( MeanShareAndBalanceConstants.TOUCH_DILATION );
+    this.valveNode.mouseArea = this.valveNode.localBounds.dilated( MeanShareAndBalanceConstants.MOUSE_AREA_DILATION );
+    this.valveNode.touchArea = this.valveNode.localBounds.dilated( MeanShareAndBalanceConstants.TOUCH_AREA_DILATION );
 
     // Valve rotation event listener
     this.valveRotationFireListener = new FireListener( {
@@ -148,8 +148,7 @@ export default class PipeNode extends Node {
         isAutoSharingProperty.set( false );
         pipeModel.isCurrentlyClickedProperty.set( false );
       },
-      //REVIEW tandem name does not match this.valveRotationFireListener
-      tandem: options.tandem.createTandem( 'fireListener' )
+      tandem: options.tandem.createTandem( 'valveRotationFireListener' )
     } );
     this.valveNode.addInputListener( this.valveRotationFireListener );
 

js/intro/view/PredictMeanNode.ts

@@ -24,8 +24,7 @@ import ShadedSphereNode from '../../../../scenery-phet/js/ShadedSphereNode.js';
 
 type SelfOptions = EmptyObjectType;
 type ParentOptions = AccessibleSliderOptions & NodeOptions;
-//REVIEW should 'focusable' and 'cursor' also be omitted?
-type PredictMeanNodeOptions = SelfOptions & StrictOmit<ParentOptions, 'pickable' | 'inputEnabled'>
+type PredictMeanNodeOptions = SelfOptions & StrictOmit<ParentOptions, 'pickable' | 'inputEnabled' | 'focusable' | 'cursor' >
 
 export default class PredictMeanNode extends AccessibleSlider( Node, 0 ) {
   private readonly predictMeanLine: Line;
@@ -85,10 +84,10 @@ export default class PredictMeanNode extends AccessibleSlider( Node, 0 ) {
   }
 
   private setPointerAreas(): void {
-    this.predictMeanLine.mouseArea = this.predictMeanLine.localBounds.dilated( MeanShareAndBalanceConstants.MOUSE_DILATION );
+    this.predictMeanLine.mouseArea = this.predictMeanLine.localBounds.dilated( MeanShareAndBalanceConstants.MOUSE_AREA_DILATION );
     this.predictMeanLine.touchArea = this.predictMeanLine.mouseArea;
-    this.predictMeanHandle.mouseArea = this.predictMeanHandle.localBounds.dilated( MeanShareAndBalanceConstants.MOUSE_DILATION );
-    this.predictMeanHandle.touchArea = this.predictMeanHandle.localBounds.dilated( MeanShareAndBalanceConstants.TOUCH_DILATION );
+    this.predictMeanHandle.mouseArea = this.predictMeanHandle.localBounds.dilated( MeanShareAndBalanceConstants.MOUSE_AREA_DILATION );
+    this.predictMeanHandle.touchArea = this.predictMeanHandle.localBounds.dilated( MeanShareAndBalanceConstants.TOUCH_AREA_DILATION );
   }
 
   private updateLine( lineEnd: number ): void {

js/intro/view/TickMarksNode.ts

@@ -16,7 +16,6 @@ import meanShareAndBalance from '../../meanShareAndBalance.js';
 type SelfOptions = EmptyObjectType;
 type TickMarksNodeOptions = SelfOptions & PickRequired<NodeOptions, 'visibleProperty' | 'tandem'>;
 
-// Should tick marks be common code?
 export default class TickMarksNode extends Node {
 
   public constructor( cupHeight: number, providedOptions: TickMarksNodeOptions ) {

js/intro/view/WaterLevelTriangleNode.ts

@@ -63,8 +63,8 @@ export default class WaterLevelTriangleNode extends Node {
     } );
 
     // Set pointer areas for slider thumb node.
-    waterLevelTriangle.mouseArea = waterLevelTriangle.localBounds.dilated( MeanShareAndBalanceConstants.MOUSE_DILATION );
-    waterLevelTriangle.touchArea = waterLevelTriangle.localBounds.dilated( MeanShareAndBalanceConstants.TOUCH_DILATION );
+    waterLevelTriangle.mouseArea = waterLevelTriangle.localBounds.dilated( MeanShareAndBalanceConstants.MOUSE_AREA_DILATION );
+    waterLevelTriangle.touchArea = waterLevelTriangle.localBounds.dilated( MeanShareAndBalanceConstants.TOUCH_AREA_DILATION );
 
     this.addChild( this.slider );
     this.mutate( options );

js/leveling-out/view/LevelingOutScreenView.ts

@@ -7,11 +7,27 @@
  * @author Sam Reid (PhET Interactive Simulations)
  */
 
-import MeanShareAndBalanceScreenView from '../../common/view/MeanShareAndBalanceScreenView.js';
+import optionize from '../../../../phet-core/js/optionize.js';
+import EmptyObjectType from '../../../../phet-core/js/types/EmptyObjectType.js';
+import MeanShareAndBalanceScreenView, { MeanShareAndBalanceScreenViewOptions } from '../../common/view/MeanShareAndBalanceScreenView.js';
 import meanShareAndBalance from '../../meanShareAndBalance.js';
+import LevelingOutModel from '../model/LevelingOutModel.js';
+
+type SelfOptions = EmptyObjectType;
+
+type LevelingOutScreenViewOptions = SelfOptions & MeanShareAndBalanceScreenViewOptions;
 
 export default class LevelingOutScreenView extends MeanShareAndBalanceScreenView {
 
+  public constructor( levelingOutModel: LevelingOutModel, providedOptions?: LevelingOutScreenViewOptions ) {
+
+    const options = optionize<LevelingOutScreenViewOptions, SelfOptions, MeanShareAndBalanceScreenViewOptions>()( {
+
+    }, providedOptions );
+
+    super( levelingOutModel, options );
+  }
+
 }
 
 meanShareAndBalance.register( 'LevelingOutScreenView', LevelingOutScreenView );