Skip to content

Commit

Permalink
More review notes, see #253
Browse files Browse the repository at this point in the history
  • Loading branch information
jonathanolson committed Mar 13, 2018
1 parent d6b8078 commit a3984fe
Show file tree
Hide file tree
Showing 9 changed files with 15 additions and 1 deletion.
1 change: 1 addition & 0 deletions js/common/model/Mass.js
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,7 @@ define( function( require ) {
this.isAnimatingProperty.set( false );
}
}

// If we're not animating/controlled or attached to a spring, we'll fall due to gravity
else if ( this.springProperty.get() === null && !this.userControlledProperty.get() ) {
var oldY = this.positionProperty.get().y;
Expand Down
1 change: 1 addition & 0 deletions js/common/model/Spring.js
Original file line number Diff line number Diff line change
Expand Up @@ -533,6 +533,7 @@ define( function( require ) {
assert && assert( !isNaN( this.massAttachedProperty.get().verticalVelocityProperty.get() ), 'velocity must be a number' );

}

// Critically damped case
else {
//TODO:: if needed decouple these objects
Expand Down
2 changes: 2 additions & 0 deletions js/common/view/DraggableTimerNode.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ define( function( require ) {
tandem: tandem.createTandem( 'positionProperty' ),
phetioType: PropertyIO( Vector2IO )
} );
//REVIEW: Use `this` instead of self here?
this.positionProperty.linkAttribute( self, 'translation' );

//REVIEW: Specify read-write as (read-only) AFTER the type docs. Add type docs here.
Expand All @@ -79,6 +80,7 @@ define( function( require ) {
}
} );
this.addInputListener( this.timerNodeMovableDragHandler );
//REVIEW: Use `this` instead of self here?
visibleProperty.linkAttribute( self, 'visible' );
}

Expand Down
1 change: 1 addition & 0 deletions js/common/view/EnergyGraphNode.js
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,7 @@ define( function( require ) {
} )
} );
}

// close it on a click
var closeListener = new ButtonListener( {
fire: dialog.hide.bind( dialog )
Expand Down
6 changes: 6 additions & 0 deletions js/common/view/MassNode.js
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ define( function( require ) {
} );

if ( mass.isLabeled ) {
//REVIEW: Use `this` instead of self here?
self.addChild( label );
}
mass.massProperty.link( function() {
Expand All @@ -145,6 +146,7 @@ define( function( require ) {

// Adjust the mass label for adjustable masses.
if ( this.mass.adjustable ) {
//REVIEW: Use `this` instead of self here?
self.mass.massProperty.link( function( massValue ) {
label.setText( StringUtils.fillIn( massValueString, { mass: Util.roundSymmetric( massValue * 1000 ) } ) );
label.center = rect.center;
Expand Down Expand Up @@ -261,15 +263,19 @@ define( function( require ) {
};

// Show/hide the velocity arrow
//REVIEW: Use `this` instead of self here?
updateArrowVisibility( model.velocityVectorVisibilityProperty, self.velocityArrow );

// Show/hide the acceleration arrow
//REVIEW: Use `this` instead of self here?
updateArrowVisibility( model.accelerationVectorVisibilityProperty, self.accelerationArrow );

// Show/hide the spring force arrow
//REVIEW: Use `this` instead of self here?
updateForceVisiblity( model.springVectorVisibilityProperty, self.springForceArrow );

// Show/hide the gravity force arrow
//REVIEW: Use `this` instead of self here?
updateForceVisiblity( model.gravityVectorVisibilityProperty, self.gravityForceArrow );

// Show/hide the net force arrow
Expand Down
1 change: 1 addition & 0 deletions js/common/view/MovableLineNode.js
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ define( function( require ) {
tandem: tandem.createTandem( 'dragHandler' )
} ) );

//REVIEW: Use `this` instead of self here?
visibleProperty.linkAttribute( self, 'visible' );

this.addChild( line );
Expand Down
1 change: 0 additions & 1 deletion js/common/view/OscillatingSpringNode.js
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,6 @@ define( function( require ) {
}
},
{
// statics
MAP_NUMBER_OF_LOOPS: MAP_NUMBER_OF_LOOPS
} );
} );
1 change: 1 addition & 0 deletions js/common/view/ReferenceLineNode.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ define( function( require ) {
self.translation = position.minus( new Vector2( LINE_LENGTH / 2, 0 ) );
} );

//REVIEW: Use `this` instead of self here?
visibleProperty.linkAttribute( self, 'visible' );

//REVIEW: Lots of similarities between this and MovableLineNode. Could be considered to be factored out.
Expand Down
2 changes: 2 additions & 0 deletions js/intro/model/IntroModel.js
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ define( function( require ) {
} );

// Manages logic for updating spring thickness and spring constant
//REVIEW: Use `this` instead of self here?
self.spring1.naturalRestingLengthProperty.link( function( naturalRestingLength ) {

//REVIEW: Style guide wants one space after //
Expand All @@ -129,6 +130,7 @@ define( function( require ) {
}
} );

//REVIEW: Use `this` instead of self here?
self.constantParameterProperty.link( function( selectedConstant ) {
// Manages logic for changing between constant parameters
// TODO: Enumerate these constants for checks
Expand Down

0 comments on commit a3984fe

Please sign in to comment.