More review for #253

js/common/model/Body.js

@@ -44,6 +44,7 @@ define( function( require ) {
   // A new tandem instance is required here since the bodies are created statically.
   var tandem = Tandem.rootTandem.createTandem( 'bodies' );
 
+  //REVIEW: Specify read-write as (read-only) AFTER the type docs. Add type docs here.
   // @public {read-only} body objects for gravity panel
   Body.MOON = new Body( bodyMoonString, MassesAndSpringsConstants.MOON_GRAVITY, tandem.createTandem( 'moon' ) );
   Body.EARTH = new Body( bodyEarthString, MassesAndSpringsConstants.EARTH_GRAVITY, tandem.createTandem( 'earth' ) );

js/common/model/Spring.js

@@ -223,6 +223,7 @@ define( function( require ) {
     );
 
     //REVIEW: NumberProperty?
+    //REVIEW: Specify read-write as (read-only) AFTER the type docs. Add type docs here.
     // @public {read-only} y position of the equilibrium position
     //REVIEW: Consider trying to make this a DerivedProperty. Also... why is its phetioType a DerivedPropertyIO?
     //REVIEW: Maybe it can be null when there is no mass attached?

js/common/view/DraggableRulerNode.js

@@ -34,14 +34,17 @@ define( function( require ) {
   function DraggableRulerNode( mvt, dragBounds, initialPosition, visibleProperty, tandem ) {
     var self = this;
 
+    //REVIEW: Specify read-write as (read-write) AFTER the type docs. Add type docs here.
     // @public {read-write} Used for returning ruler to toolbox. Set this if needed to be returned.
     this.toolbox = null;
 
     // define ruler params in pixels
+    //REVIEW: How is converting 0.5 through the MVT 1 meter? Is it now 0.5 meters?
     var rulerLength = mvt.modelToViewY( .5 ); // 1 meter
     var rulerWidth = 0.125 * rulerLength;
     var majorTickLabels = [ '' ];
     for ( var i = 1; i < 10; i++ ) {
+      //REVIEW: Why the Math.floor usage on what seems to be whole integers? Should be no-ops?
       majorTickLabels.push( '' );
       majorTickLabels.push( '' + Math.floor( i * 10 ) );
       assert && assert( majorTickLabels[ i * 2 ] === '' + Math.floor( i * 10 ) );
@@ -66,6 +69,7 @@ define( function( require ) {
     }, { tandem: tandem.createTandem( 'ruler' ) } );
 
     //REVIEW: Type docs
+    //REVIEW: Specify read-write as (read-only) AFTER the type docs. Add type docs here.
     // @private {read-only} position of ruler node in screen coordinates
     this.positionProperty = new Property( initialPosition, {
       tandem: tandem.createTandem( 'positionProperty' ),
@@ -74,6 +78,7 @@ define( function( require ) {
     this.positionProperty.linkAttribute( this, 'translation' );
 
     //REVIEW: Type docs
+    //REVIEW: Specify read-write as (read-only) AFTER the type docs. Add type docs here.
     // @private {read-only} handles ruler node drag events
     this.rulerNodeMovableDragHandler = new MovableDragHandler( this.positionProperty, {
       tandem: tandem.createTandem( 'dragHandler' ),
@@ -104,6 +109,7 @@ define( function( require ) {
       this.positionProperty.reset();
     },
 
+    //REVIEW: JSDoc. Also why not use a Property.<Bounds2> so we don't need this method?
     updateBounds: function( newBounds ) {
       this.rulerNodeMovableDragHandler.dragBounds = newBounds;
     }

js/common/view/DraggableTimerNode.js

@@ -44,17 +44,20 @@ define( function( require ) {
       tandem: tandem.createTandem( 'timer' )
     } );
 
+    //REVIEW: Specify read-write as (read-write) AFTER the type docs. Add type docs here.
     // @public {read-write} Used for returning ruler to toolbox. Set this if needed to be returned.
     this.toolbox = null;
 
     //REVIEW: type docs
+    //REVIEW: Specify read-write as (read-only) AFTER the type docs. Add type docs here.
     // @private {read-only} position of ruler node in screen coordinates
     this.positionProperty = new Property( initialPosition, {
       tandem: tandem.createTandem( 'positionProperty' ),
       phetioType: PropertyIO( Vector2IO )
     } );
     this.positionProperty.linkAttribute( self, 'translation' );
 
+    //REVIEW: Specify read-write as (read-only) AFTER the type docs. Add type docs here.
     // @private {read-only} handles timer node drag events
     this.timerNodeMovableDragHandler = new MovableDragHandler( this.positionProperty, {
       tandem: tandem.createTandem( 'dragHandler' ),

js/common/view/EnergyGraphNode.js

@@ -71,6 +71,7 @@ define( function( require ) {
       var MIN_SCALE = 1;
       var MAX_SCALE = 32;
 
+      //REVIEW: Specify read-write as (read-write) AFTER the type docs. Add type docs here. Adn visibility
       // {read-write} Responsible for the zoom level in the bar graph. Is adjusted by the zoom buttons and used for the
       // scaling property of the barNodes.
       //REVIEW: JSDoc?
@@ -99,6 +100,7 @@ define( function( require ) {
         self.zoomLevelProperty.value += 1;
       } );
 
+      //REVIEW: Doesn't need read-write, since it's local. Also it should be read-only (DerivedProperty)
       // {read-write} Responsible for adjusting the scaling of the barNode heights.
       var scaleFactorProperty = new DerivedProperty( [ this.zoomLevelProperty ], function( zoomLevel ) {
         return Math.pow( 2, zoomLevel ) * 20;

js/common/view/GravityAndDampingControlNode.js

@@ -78,6 +78,7 @@ define( function( require ) {
     // @public
     this.bodyProperty = model.bodyProperty;
 
+    //REVIEW: Does not need visibility/read-only docs, as it's a local variable
     // @private {read-only} manages the items associated with the gravity panel in a combo box
     var gravityComboBox = new ComboBox( bodyListItems, model.bodyProperty, listNodeParent, {
       buttonCornerRadius: 3,

js/common/view/MassNode.js

@@ -62,6 +62,8 @@ define( function( require ) {
     } );
     this.addChild( rect );
 
+    //REVIEW: Doesn't need read-write docs, as it's a local variable. Also it should be read-only (if ever doc'ed)
+    //REVIEW: since it is a DerivedProperty and can't be set
     // {read-write} Bounds that limit where we can drag our mass should be dependent on how large our mass is
     var modelBoundsProperty = new DerivedProperty( [ dragBoundsProperty, mass.heightProperty ], function( dragBounds, massHeight ) {
       var modelBounds = modelViewTransform2.viewToModelBounds( dragBounds );
@@ -147,6 +149,7 @@ define( function( require ) {
       } );
     }
 
+    //REVIEW: Specify read-write as (read-write) AFTER the type docs. Add type docs here.
     // @public {read-write}
     this.movableDragHandler = new MovableDragHandler( this.mass.positionProperty, {
 

js/common/view/MovableLineNode.js

@@ -61,16 +61,19 @@ define( function( require ) {
     line.mouseArea = dilatedLineBounds;
     line.touchArea = dilatedLineBounds;
 
+    //REVIEW: This is a method call, visibility and read-only don't make sense on a setter method.
     // @private {read-only} X coordinate for the position of the line
     initialPosition.setX( dragBounds.minX );
 
     //REVIEW: type docs
+    //REVIEW: Specify read-write as (read-write) AFTER the type docs. Add type docs here.
     // @private {read-write} position of line in screen coordinates
     this.positionProperty = new Property( initialPosition, {
       tandem: tandem.createTandem( 'positionProperty' ),
       phetioType: PropertyIO( Vector2IO )
     } );
 
+    //REVIEW: Specify read-write as (read-write) AFTER the type docs. Add type docs here.
     // @private {read-write} position of line in screen coordinates
     this.positionProperty.link( function( position ) {
       self.translation = position.minus( new Vector2( length / 2, 0 ) );

js/common/view/ReferenceLineNode.js

@@ -47,13 +47,16 @@ define( function( require ) {
     this.mouseArea = this.localBounds.dilatedY( 10 );
     this.touchArea = this.localBounds.dilatedY( 10 );
 
+    //REVIEW: Does not need read-write since it's a local variable
     // {read-write} prevents overlap with the equilibrium line
     var xPos = modelViewTransform2.modelToViewX( spring.positionProperty.get().x );
 
     // Helper function to derive the length as if the mass wasn't attached.
     var lengthFunction = new LinearFunction( 0.1, 0.5, 1.37, 0.97 );
 
     var yPos = modelViewTransform2.modelToViewY( lengthFunction( spring.naturalRestingLengthProperty.value ) );
+
+    //REVIEW: Specify read-write as (read-write) AFTER the type docs. Add type docs here.
     // @private {read-write} position of line in screen coordinates.
     this.positionProperty = new Property( new Vector2( xPos, yPos ), {
       phetioType: PropertyIO( Vector2IO )

js/intro/model/IntroModel.js

@@ -64,9 +64,11 @@ define( function( require ) {
     } );
 
     // initial parameters set for both scenes
+    //REVIEW: Specify read-write as (read-write) AFTER the type docs. Add type docs here.
     // @private {read-write} array of parameters for scene 1
     var sameLengthModeSpringState = this.getSpringState();
 
+    //REVIEW: Specify read-write as (read-write) AFTER the type docs. Add type docs here.
     // @private {read-write} array of parameters for scene 2
     this.spring1.naturalRestingLengthProperty.set( MassesAndSpringsConstants.DEFAULT_SPRING_LENGTH / 2 );
     var adjustableLengthModeSpringState = this.getSpringState();

js/intro/view/IntroScreenView.js

@@ -183,12 +183,15 @@ define( function( require ) {
       springOptionsPanel.visible = self.springLengthControlPanel.visible;
     } );
 
+    //REVIEW: Local variables shouldn't have visibility docs, and don't need read-only docs.
     // @private {read-only} Creation of same length icon node
     var sameLengthIcon = new SceneSelectionButton( 'same-length', this.modelViewTransform, tandem );
 
+    //REVIEW: Local variables shouldn't have visibility docs, and don't need read-only docs.
     // @private {read-only} Creation of adjustable length icon node
     var differentLengthIcon = new SceneSelectionButton( 'adjustable-length', this.modelViewTransform, tandem );
 
+    //REVIEW: Local variables shouldn't have visibility docs, and don't need read-only docs.
     // @private {read-only} Creation of toggled modes for scene selection
     var toggleButtonsContent = [ {
       value: 'same-length',
@@ -198,6 +201,7 @@ define( function( require ) {
       node: differentLengthIcon
     } ];
 
+    //REVIEW: Local variables shouldn't have visibility docs, and don't need read-only docs.
     // @private {read-only} Creation of icons for scene selection
     var sceneRadioButtonGroup = new RadioButtonGroup( model.sceneModeProperty, toggleButtonsContent, {
       buttonContentXMargin: 1,

js/intro/view/SceneSelectionButton.js

@@ -35,6 +35,7 @@ define( function( require ) {
       opacity: 0.9
     } );
 
+    //REVIEW: Local variables shouldn't have visibility docs, and don't need read-only docs.
     // @public {read-only} Springs created to be used in the icons for the scene selection tabs
     var springsIcon = [
       new Spring(
@@ -51,6 +52,7 @@ define( function( require ) {
       )
     ];
 
+    //REVIEW: Local variables shouldn't have visibility docs, and don't need read-only docs.
     // @private {read-only} Creation of spring for use in scene switching icons
     var springNodeOptions = {
       frontColor: '#000000',
@@ -66,6 +68,7 @@ define( function( require ) {
     firstSpringIcon.loopsProperty.set( 6 );
     firstSpringIcon.lineWidthProperty.set( 3 );
 
+    //REVIEW: Local variables shouldn't have visibility docs, and don't need read-only docs.
     // @private {read-only} Creation of spring for use in scene switching icons
     var secondSpringIcon = new OscillatingSpringNode(
       springsIcon[ 1 ],
@@ -104,6 +107,7 @@ define( function( require ) {
       opacity: this.opacity
     } );
 
+    //REVIEW: Local variables shouldn't have visibility docs, and don't need read-only docs.
     // @private {read-only} White background for scene switching icons
     var iconBackground = new Rectangle( firstSpringIcon.left - 20, -170, 180, 170, 10, 10, {
       fill: '#A8D2FF'