Add review comments, see #52

js/common/MeanShareAndBalanceConstants.ts

@@ -54,6 +54,8 @@ const MeanShareAndBalanceConstants = {
 
   PLATE_CHOCOLATE_CENTER_Y: 310,
   PEOPLE_CENTER_Y: 500,
+
+  // REVIEW: Should this be called MAX_NUMBER_OF_CHOCOLATES_PER_PERSON?
   MAX_NUMBER_OF_CHOCOLATES: 10,
   MIN_NUMBER_OF_CHOCOLATES: 0,
   NOTEBOOK_PAPER_CENTER_Y: 220

js/common/view/NoteBookPaperNode.ts

@@ -36,10 +36,12 @@ export default class NoteBookPaperNode extends Node {
     const rings = [];
     const numberOfRings = 8;
 
+    // REVIEW: _.times looks like a good match here
     for ( let i = 0; i < numberOfRings; i++ ) {
       const x = i * ( ( paperWidth - 20 ) / numberOfRings ) + 30;
 
-      rings.push( new Image( notepadRing_png, { x: x, y: -21.5, scale: 0.8 } ) );
+      const image = new Image( notepadRing_png, { x: x, y: -21.5, scale: 0.8 } );
+      rings.push( image );
     }
 
     super( { children: [ paperStackNode, ...rings ], centerY: MeanShareAndBalanceConstants.NOTEBOOK_PAPER_CENTER_Y } );

js/leveling-out/view/LevelingOutScreenView.ts

@@ -147,7 +147,11 @@ export default class LevelingOutScreenView extends MeanShareAndBalanceScreenView
       children: [ ...tablePlatesNodes, ...paperPlatesNodes, ...draggableChocolateBars ]
     } );
 
-    const meanCalculationDialog = new MeanCalculationDialog( model.peopleArray, model.meanCalculationDialogVisibleProperty );
+    const meanCalculationDialog = new MeanCalculationDialog(
+      model.peopleArray,
+      model.meanCalculationDialogVisibleProperty,
+      notebookPaper.bounds
+    );
 
     // REVIEW: optionize?
     const combinedOptions = combineOptions<ScreenViewOptions>( { children: [ notebookPaper, peopleLayerNode, tableNode, chocolateLayerNode ] }, options );

js/leveling-out/view/MeanCalculationDialog.ts

@@ -5,7 +5,6 @@
  *
  * @author Marla Schulz (PhET Interactive Simulations)
  * @author Sam Reid (PhET Interactive Simulations)
- *
  */
 
 import Dialog from '../../../../sun/js/Dialog.js';
@@ -17,41 +16,36 @@ import Multilink from '../../../../axon/js/Multilink.js';
 import MeanShareAndBalanceConstants from '../../common/MeanShareAndBalanceConstants.js';
 import Property from '../../../../axon/js/Property.js';
 import Panel from '../../../../sun/js/Panel.js';
-import NoteBookPaperNode from '../../common/view/NoteBookPaperNode.js';
 import MeanShareAndBalanceStrings from '../../MeanShareAndBalanceStrings.js';
-
-const NOTEBOOK_PAPER_NODE = new NoteBookPaperNode();
-
+import Bounds2 from '../../../../dot/js/Bounds2.js';
 
 export default class MeanCalculationDialog extends Dialog {
 
-  public constructor( people: Array<Person>, visibleProperty: Property<boolean> ) {
-
-    const isActiveProperties = people.map( person => person.isActiveProperty );
-    const numberOfChocolatesProperties = people.map( person => person.chocolateNumberProperty );
+  public constructor( people: Array<Person>, visibleProperty: Property<boolean>, notebookPaperBounds: Bounds2 ) {
 
     const meanTitleText = new Text( MeanShareAndBalanceStrings.meanStringProperty );
     const meanEqualsAdditionFractionText = new Text( MeanShareAndBalanceStrings.meanEqualsStringProperty );
     const meanEqualsFractionText = new Text( MeanShareAndBalanceStrings.meanEqualsStringProperty );
     const meanEqualsDecimalText = new Text( MeanShareAndBalanceStrings.meanEqualsStringProperty );
 
-
     const calculationNode = new GridBox( {
       margin: 10
     } );
 
-    const notebookPaperWidth = NOTEBOOK_PAPER_NODE.width;
-    const notebookPaperHeight = NOTEBOOK_PAPER_NODE.height;
-
     const panel = new Panel( calculationNode, {
       stroke: null,
-      minWidth: notebookPaperWidth - 76.4, // the left and right margin calculated by Dialog.ts
-      minHeight: notebookPaperHeight - 40 // the top/bottom margin, and y spacing implemented by Dialog.ts
+      minWidth: notebookPaperBounds.width - 76.4, // the left and right margin calculated by Dialog.ts
+      minHeight: notebookPaperBounds.height - 40 // the top/bottom margin, and y spacing implemented by Dialog.ts
     } );
 
+    const isActiveProperties = people.map( person => person.isActiveProperty );
+    const numberOfChocolatesProperties = people.map( person => person.chocolateNumberProperty );
     Multilink.multilinkAny( [ ...isActiveProperties, ...numberOfChocolatesProperties ], () => {
       const numbers = people.filter( person => person.isActiveProperty.value ).map( person => person.chocolateNumberProperty.value );
       const numberOfPeople = people.filter( person => person.isActiveProperty.value ).length;
+
+      // REVIEW: Check with JB about whether this should be i18nized?
+      // REVIEW: Can we align the numbers with the table spinners?  So correspondence is clear?
       const additionText = new Text( numbers.join( ' + ' ) );
       const additionFractionLine = new Line( 0, 0, additionText.width, 0, { stroke: 'black' } );
       const additionDenominatorText = new Text( numberOfPeople );
@@ -64,24 +58,25 @@ export default class MeanCalculationDialog extends Dialog {
 
       const decimalText = new Text( Utils.toFixedNumber( _.sum( numbers ) / numberOfPeople, 2 ) );
 
-      calculationNode.rows = [ [ meanEqualsAdditionFractionText, additionFraction ], [ meanEqualsFractionText, fraction ], [ meanEqualsDecimalText, decimalText ] ];
+      calculationNode.rows = [
+        [ meanEqualsAdditionFractionText, additionFraction ],
+        [ meanEqualsFractionText, fraction ],
+        [ meanEqualsDecimalText, decimalText ]
+      ];
     } );
 
-
     super( panel, {
-        title: meanTitleText,
-        titleAlign: 'left',
-        visibleProperty: visibleProperty,
-        resize: false,
-        centerY: MeanShareAndBalanceConstants.NOTEBOOK_PAPER_CENTER_Y,
-        closeButtonListener: () => { this.visibleProperty.set( false ); },
-
-        // We specify the position manually
-        // REVIEW: Where is x specified?
-        layoutStrategy: _.noop
-      }
-    );
-
+      title: meanTitleText,
+      titleAlign: 'left',
+      visibleProperty: visibleProperty,
+      resize: false,
+      centerY: MeanShareAndBalanceConstants.NOTEBOOK_PAPER_CENTER_Y,
+      closeButtonListener: () => this.visibleProperty.set( false ),
+
+      // We specify the position manually
+      // REVIEW: Where is x specified?
+      layoutStrategy: _.noop
+    } );
   }
 }
 

js/leveling-out/view/PaperPlateNode.ts

@@ -1,7 +1,7 @@
 // Copyright 2022, University of Colorado Boulder
 
 /**
- * Contains all the chocolate bars on a plate. Each plate has one PaperPlateNode,
+ * In the upper (paper) representation, contains all the chocolate bars on a plate. Each plate has one PaperPlateNode,
  * and each container has a maximum of 10 chocolates bars.
  *
  * @author Marla Schulz (PhET Interactive Simulations)
@@ -18,21 +18,18 @@ import DraggableChocolate from './DraggableChocolate.js';
 import PickRequired from '../../../../phet-core/js/types/PickRequired.js';
 import MeanShareAndBalanceConstants from '../../common/MeanShareAndBalanceConstants.js';
 
-type ChocolateBarsContainerNodeOptions = StrictOmit<VBoxOptions, keyof NodeTranslationOptions> & PickRequired<NodeOptions, 'tandem'>;
+type ChocolateBarsContainerNodeOptions = StrictOmit<VBoxOptions, keyof NodeTranslationOptions | 'children'> & PickRequired<NodeOptions, 'tandem'>;
 
 export default class PaperPlateNode extends Node {
   public constructor( plate: Plate, chocolateBarDropped: ( chocolateBar: DraggableChocolate ) => void, providedOptions: ChocolateBarsContainerNodeOptions ) {
     const options = optionize<ChocolateBarsContainerNodeOptions, EmptySelfOptions, NodeOptions>()( {
       x: plate.position.x,
       y: plate.position.y,
-      visibleProperty: plate.isActiveProperty
+      visibleProperty: plate.isActiveProperty,
+      excludeInvisibleChildrenFromBounds: false,
+      children: [ new Line( 0, 0, MeanShareAndBalanceConstants.CHOCOLATE_WIDTH, 0, { stroke: 'black' } ) ]
     }, providedOptions );
 
-    const plateLine = new Line( 0, 0, MeanShareAndBalanceConstants.CHOCOLATE_WIDTH, 0, { stroke: 'black' } );
-
-
-    options.children = [ plateLine ];
-    options.excludeInvisibleChildrenFromBounds = false;
     super( options );
   }
 }

js/leveling-out/view/PersonImage.ts

@@ -21,10 +21,6 @@ export default class PersonImage extends Image {
   public constructor( image: HTMLImageElement, plate: TablePlateNode, providedOptions: PersonImageOptions ) {
     const options = optionize<PersonImageOptions, SelfOptions, ImageOptions>()( {
       scale: 0.3,
-
-      // REVIEW: It seems somewhat circular that the Person coordinates determine where to draw the TablePlateNode,
-      // then the TablePlateNode determines where to draw the PersonImage
-      // MS:
       centerX: plate.centerX,
       bottom: plate.bottom - 55
     }, providedOptions );

js/leveling-out/view/TablePlateNode.ts

@@ -41,11 +41,10 @@ export default class TablePlateNode extends Node {
     const chocolateScale = 0.04;
 
     // create chocolate person brought
-    const chocolatesArray: Array<Image> = [];
-    for ( let i = 0; i < MeanShareAndBalanceConstants.MAX_NUMBER_OF_CHOCOLATES; i++ ) {
-      const chocolate = new Image( chocolateBar_png, { scale: chocolateScale } );
-      chocolatesArray.push( chocolate );
-    }
+    // REVIEW: See if it would be appropriate to use _.times elsewhere
+    const chocolatesArray = _.times( MeanShareAndBalanceConstants.MAX_NUMBER_OF_CHOCOLATES, () => new Image( chocolateBar_png, {
+      scale: chocolateScale
+    } ) );
 
     const chocolatesVBox = new VBox( {
       children: chocolatesArray,

js/mean-share-and-balance-main.ts

@@ -21,9 +21,9 @@ const simOptions: SimOptions = {
     softwareDevelopment: 'Marla Schulz, Sam Reid',
     team: 'Kelly Findley, Marilyn Hartzell, Ariel Paul, Kathy Perkins, David Webb',
     qualityAssurance: 'Clifford Hardin, Emily Miller, Nancy Salpepi, Kathryn Woessner',
-    graphicArts: 'Mariah Hermsmeyer',
-    soundDesign: '',
-    thanks: ''
+    graphicArts: 'Mariah Hermsmeyer'
+    // soundDesign: '',
+    // thanks: ''
   }
 };