Addressed REVIEWS: Responding to REVIEW comments.

phetsims/masses-and-springs-basics#48
phetsims · Feb 1, 2019 · 5e7c382 · 5e7c382
1 parent 67ad3a1
commit 5e7c382
Show file tree

Hide file tree

Showing 2 changed files with 4 additions and 0 deletions.
diff --git a/js/common/view/DraggableTimerNode.js b/js/common/view/DraggableTimerNode.js
@@ -53,6 +53,8 @@ define( function( require ) {
     } );
     this.positionProperty.linkAttribute( this, 'translation' );
     // REVIEW: Also seems like a lot of code duplicated with DraggableRulerNode. Is there anything that can be deduplicated?
+    // *REVIEW: Similarities are the use of a position property and the start/end callbacks of the MovableDragHandler.
+    // *REVIEW: I'm don't mind the duplication here because it is readable.
 
     // @private {MovableDragHandler} (read-only) handles timer node drag events
     this.timerNodeMovableDragHandler = new MovableDragHandler( this.positionProperty, {

diff --git a/js/intro/view/ConstantsControlPanel.js b/js/intro/view/ConstantsControlPanel.js
@@ -60,6 +60,8 @@ define( function( require ) {
 
     var constantText = new Text(
       // REVIEW: Why the fill in with an empty string? Looks suspicious
+      // *REVIEW: SpringConstant is a string that is used elsewhere but has a placeholder.
+      // *REVIEW: Alternatively, I can create a springConstantString without a placeholder.
       StringUtils.fillIn( springConstantString, { spring: '' } ),
       _.extend( { font: TITLE_FONT, tandem: tandem.createTandem( 'constantText' ) },
         constantsSelectionButtonOptions ) );