diff --git a/js/common/MeanShareAndBalanceConstants.ts b/js/common/MeanShareAndBalanceConstants.ts index 42354a2b..6dc3a6b4 100644 --- a/js/common/MeanShareAndBalanceConstants.ts +++ b/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 diff --git a/js/common/view/MeanShareAndBalanceScreenView.ts b/js/common/view/MeanShareAndBalanceScreenView.ts index b7c246c8..81fd4dd6 100644 --- a/js/common/view/MeanShareAndBalanceScreenView.ts +++ b/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()( {}, 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 ); \ No newline at end of file diff --git a/js/intro/model/IntroModel.ts b/js/intro/model/IntroModel.ts index b0ca7701..984e195b 100644 --- a/js/intro/model/IntroModel.ts +++ b/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 } diff --git a/js/intro/model/PipeModel.ts b/js/intro/model/PipeModel.ts index a0846722..a9f79f6c 100644 --- a/js/intro/model/PipeModel.ts +++ b/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; diff --git a/js/intro/view/IntroScreenView.ts b/js/intro/view/IntroScreenView.ts index efe99f52..730a0120 100644 --- a/js/intro/view/IntroScreenView.ts +++ b/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,7 +222,7 @@ export default class IntroScreenView extends MeanShareAndBalanceScreenView { predictMeanLine, numberOfCupsNumberSpinner, waterCupLayerNode, - this.syncDataButton + this.syncRepresentationsButton ]; this.pdomControlAreaNode.pdomOrder = [ @@ -232,8 +230,8 @@ export default class IntroScreenView extends MeanShareAndBalanceScreenView { ]; } - //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 ); } diff --git a/js/intro/view/PipeNode.ts b/js/intro/view/PipeNode.ts index beaf0300..e9f1910d 100644 --- a/js/intro/view/PipeNode.ts +++ b/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 ); diff --git a/js/intro/view/PredictMeanNode.ts b/js/intro/view/PredictMeanNode.ts index 18ef1629..2b5ee51d 100644 --- a/js/intro/view/PredictMeanNode.ts +++ b/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 +type PredictMeanNodeOptions = SelfOptions & StrictOmit 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 { diff --git a/js/intro/view/TickMarksNode.ts b/js/intro/view/TickMarksNode.ts index 5df7fe1d..b4dce380 100644 --- a/js/intro/view/TickMarksNode.ts +++ b/js/intro/view/TickMarksNode.ts @@ -16,7 +16,6 @@ import meanShareAndBalance from '../../meanShareAndBalance.js'; type SelfOptions = EmptyObjectType; type TickMarksNodeOptions = SelfOptions & PickRequired; -// Should tick marks be common code? export default class TickMarksNode extends Node { public constructor( cupHeight: number, providedOptions: TickMarksNodeOptions ) { diff --git a/js/intro/view/WaterLevelTriangleNode.ts b/js/intro/view/WaterLevelTriangleNode.ts index ffe84348..9ae9d013 100644 --- a/js/intro/view/WaterLevelTriangleNode.ts +++ b/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 ); diff --git a/js/leveling-out/view/LevelingOutScreenView.ts b/js/leveling-out/view/LevelingOutScreenView.ts index 2e8732af..872a0a18 100644 --- a/js/leveling-out/view/LevelingOutScreenView.ts +++ b/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()( { + + }, providedOptions ); + + super( levelingOutModel, options ); + } + } meanShareAndBalance.register( 'LevelingOutScreenView', LevelingOutScreenView ); \ No newline at end of file