Address review comments for documentation, see #247

phetsims · Aug 29, 2019 · b6a6c3f · b6a6c3f
1 parent 50e41b2
commit b6a6c3f
Show file tree

Hide file tree

Showing 4 changed files with 14 additions and 11 deletions.
diff --git a/js/systems/model/Cloud.js b/js/systems/model/Cloud.js
@@ -38,7 +38,8 @@ define( require => {
       // @private {number} - used to calculate this cloud's position
       this.parentPositionProperty = parentPositionProperty;
 
-      //REVIEW #247 missing visibility annotation, type expression, description of null
+      // @private {Shape|null} - the ellipse that defines the shape of this cloud. only null until the parent position
+      // is linked
       this.cloudEllipse = null;
 
       this.parentPositionProperty.link( parentPosition => {

diff --git a/js/systems/model/EnergyChunkPathMover.js b/js/systems/model/EnergyChunkPathMover.js
@@ -40,10 +40,10 @@ define( require => {
       this.nextPoint = path[ 0 ];
     }
 
-    //REVIEW #247 missing visibility annotation
     /**
      * advance chunk position along the path
      * @param  {number} dt - time step in seconds
+     * @public
      */
     moveAlongPath( dt ) {
 

diff --git a/js/systems/model/EnergySource.js b/js/systems/model/EnergySource.js
@@ -39,10 +39,9 @@ define( require => {
       return this.outgoingEnergyChunks.splice( 0 );
     }
 
-    //REVIEW #247 this was @protected in the superclass, is @public accurate?
     /**
      * clear internal list of energy chunks and outgoing energy chunks
-     * @public
+     * @protected
      * @override
      */
     clearEnergyChunks() {

diff --git a/js/systems/model/EnergySystemElementCarousel.js b/js/systems/model/EnergySystemElementCarousel.js
@@ -26,7 +26,7 @@ define( require => {
   const Vector2 = require( 'DOT/Vector2' );
 
   // constants
-  const TRANSITION_DURATION = 0.75; //REVIEW #247 units?
+  const TRANSITION_DURATION = 0.75; // in seconds
 
   class EnergySystemElementCarousel {
 
@@ -59,9 +59,9 @@ define( require => {
       this.animationInProgressProperty = new BooleanProperty( false );
 
       // @private - variables needed to manage carousel transitions
-      this.elapsedTransitionTime = 0; //REVIEW #247 units?
-      this.currentCarouselOffset = new Vector2( 0, 0 );
-      this.initialCarouselOffset = new Vector2( 0, 0 );
+      this.elapsedTransitionTime = 0; // in seconds
+      this.currentCarouselOffset = new Vector2( 0, 0 ); // in meters
+      this.initialCarouselOffset = new Vector2( 0, 0 ); // in meters
 
       // set up the variables needed for animation each time the target changes
       this.targetIndexProperty.lazyLink( () => {
@@ -132,10 +132,10 @@ define( require => {
       return null;
     }
 
-    //REVIEW #247 missing visibility annotation
     /**
      * step this model element
      * @param {number} dt - time step, in seconds
+     * @public
      */
     step( dt ) {
       if ( !this.atTargetPosition() ) {
@@ -158,8 +158,8 @@ define( require => {
       }
     }
 
-    //REVIEW #247 document
     /**
+     * sets the position in model space of each element based on the state of the carousel
      * @private
      */
     updateManagedElementPositions() {
@@ -170,8 +170,9 @@ define( require => {
       }
     }
 
-    //REVIEW #247 document
     /**
+     * sets the opacity of each element based on the state of the carousel. elements fade out as they get farther from
+     * the active, selected position
      * @private
      */
     updateManagedElementOpacities() {
@@ -184,7 +185,9 @@ define( require => {
 
     //REVIEW #247 document
     /**
+     * whether the current selected element is in its destination spot
      * @private
+     * @returns {boolean}
      */
     atTargetPosition() {
       const targetCarouselOffset = this.offsetBetweenElements.times( -this.targetIndexProperty.value );