From 5e7c382e037175e7b206bc1b306a71d4b87defd2 Mon Sep 17 00:00:00 2001 From: denz1994 Date: Fri, 1 Feb 2019 12:20:16 -0500 Subject: [PATCH] Addressed REVIEWS: Responding to REVIEW comments. https://github.com/phetsims/masses-and-springs-basics/issues/48 --- js/common/view/DraggableTimerNode.js | 2 ++ js/intro/view/ConstantsControlPanel.js | 2 ++ 2 files changed, 4 insertions(+) diff --git a/js/common/view/DraggableTimerNode.js b/js/common/view/DraggableTimerNode.js index addf1c40..cee2e4c8 100644 --- 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 index 0e4c853d..b6a50032 100644 --- 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 ) );