Adding more REVIEW comments #63

phetsims · Aug 29, 2018 · 10d2da4 · 10d2da4
1 parent 0d26abc
commit 10d2da4
Show file tree

Hide file tree

Showing 4 changed files with 11 additions and 0 deletions.
diff --git a/js/atomic/model/CoulombsLawAtomicModel.js b/js/atomic/model/CoulombsLawAtomicModel.js
@@ -60,6 +60,7 @@ define( function( require ) {
     var rightBoundary = ISLCConstants.RIGHT_OBJECT_BOUNDARY * 1E-11;
 
     // @public - the position of the ruler in the model
+    // REVIEW: Doc type
     this.rulerPositionProperty = new Property( new Vector2( 0, -0.75E-11 ), {
       tandem: tandem.createTandem( 'rulerPositionProperty' ),
       phetioType: PropertyIO( Vector2IO )

diff --git a/js/atomic/view/CoulombsLawAtomicView.js b/js/atomic/view/CoulombsLawAtomicView.js
@@ -126,6 +126,7 @@ define( function( require ) {
         tandem: tandem.createTandem( 'atomicLegendNode' ) 
       } );
 
+    // REVIEW: Can we move this into the options at the initial declaration?
     legendNode.left = this.layoutBounds.minX + 9.35;
 
     this.addChild( legendNode );
@@ -140,12 +141,15 @@ define( function( require ) {
       tandem: tandem.createTandem( 'picometerScaleString' )
     } );
 
+    // REVIEW: Can we move this into the options at the initial declaration?
     picometerScaleNode.left = legendNode.right + 10;
 
     this.addChild( picometerScaleNode );
 
     // a11y - charges are first in focus order
     var charges = [ chargeNode1, chargeNode2 ];
+
+    // REVIEW: Visibility annotation? Doc type?
     this.accessibleOrder = charges.concat( this.accessibleOrder );
   }
 

diff --git a/js/common/model/Charge.js b/js/common/model/Charge.js
@@ -37,6 +37,7 @@ define( function( require ) {
       constantRadius: 6.75E-3 // ensure this is in meters (0.675cm)
     }, options );
 
+    // REVIEW: Consider using BooleanProperty
     var constantRadiusProperty = new Property( true, {
       tandem: tandem.createTandem( 'constantRadiusProperty' ),
       phetioType: PropertyIO( BooleanIO )
@@ -50,6 +51,7 @@ define( function( require ) {
     // @public - object node color is will change with value
     // color property will be updated based on a boolean value (negative vs positive)
     // brightness will be set according to the Mass/Charge magnitude
+    // REVIEW: Visibility annotation? Type Doc?
     this.baseColorProperty = new DerivedProperty( [ this.valueProperty ], function( value ) {
         var newBaseColor = value < 0 ? negativeColor : positiveColor;
         return newBaseColor.colorUtilsBrighter( 1 - Math.abs( value ) / valueRange.max );
@@ -64,6 +66,8 @@ define( function( require ) {
 
     // @override
     // Returns the {number} radius of the charge object
+    // REVIEW: jsDoc should have @return {type}. Eliminates need for {number} in function description.
+    // REVIEW: Function description should be first in jsDoc.
     calculateRadius: function( charge ) {
       return this.radiusProperty.get();
     },

diff --git a/js/macro/view/CoulombsLawMacroView.js b/js/macro/view/CoulombsLawMacroView.js
@@ -22,6 +22,8 @@ define( function( require ) {
   var charge1AbbreviatedString = require( 'string!COULOMBS_LAW/charge1Abbreviated' );
   var charge2AbbreviatedString = require( 'string!COULOMBS_LAW/charge2Abbreviated' );
   var unitsMacroLegendScaleString = require( 'string!COULOMBS_LAW/units.macroLegendScale' );
+
+  // REVIEW: Add microcoulombs to units section of MathSymbols.js
   var unitsMicrocoulombsString = require( 'string!COULOMBS_LAW/units.microcoulombs' );
 
   // constants