Skip to content

Commit

Permalink
More review notes, see #253
Browse files Browse the repository at this point in the history
jonathanolson committed Mar 13, 2018
1 parent ff0be56 commit 7eb4b9f
Showing 6 changed files with 35 additions and 7 deletions.
1 change: 1 addition & 0 deletions js/common/model/MassesAndSpringsModel.js
Original file line number Diff line number Diff line change
@@ -50,6 +50,7 @@ define( function( require ) {
} );

//@public {Property.<boolean>} determines whether the sim is playing sound
//REVIEW: Looks like nothing is listening to this? Can it be removed?
this.isSoundEnabledProperty = new BooleanProperty( true, {
tandem: tandem.createTandem( 'isSoundEnabledProperty' )
} );
2 changes: 1 addition & 1 deletion js/common/view/OneSpringScreenView.js
Original file line number Diff line number Diff line change
@@ -43,7 +43,7 @@ define( function( require ) {
*/
function OneSpringScreenView( model, tandem ) {
//REVIEW: JSDOc
this.model = model; // Make model available
this.model = model; // Make model available REVIEW: This is already done in the supertype, not necessary here
SpringScreenView.call( this, model, tandem );
var self = this;

24 changes: 22 additions & 2 deletions js/common/view/SpringScreenView.js
Original file line number Diff line number Diff line change
@@ -70,13 +70,16 @@ define( function( require ) {
// @protected {number} - Spacing used for the margin of layout bounds
this.spacing = 10;

// @protected {number} - Alignment for panels on most right side of sim view
// @protected {number} - Alignment for panels on most right side of sim view REVIEW: 'alignment' is usually like right/left/center. This is a padding between the right side of the screen and the panels, correct?
this.rightPanelAlignment = this.visibleBoundsProperty.get().right - this.spacing;

// Add masses
//REVIEW: JSDoc
this.massLayer = new Node( { tandem: tandem.createTandem( 'massLayer' ), preventFit: true } );
//REVIEW: JSDoc
this.massNodes = [];

//REVIEW: Could use `this.massNodes = model.masses.map( ... )`, returning the mass node at the end of the callback?
model.masses.forEach( function( mass ) {
var massNode = new MassNode(
mass,
@@ -91,6 +94,7 @@ define( function( require ) {
} );

// Add shelf for to house massNodes
//REVIEW: JSDoc
this.shelf = new Shelf( tandem, {
rectHeight: 7
} );
@@ -99,6 +103,7 @@ define( function( require ) {
this.addChild( this.shelf );

// Timer and Ruler
//REVIEW: JSDoc
this.timerNode = new DraggableTimerNode(
this.visibleBoundsProperty.get(),
Vector2.ZERO,
@@ -107,6 +112,7 @@ define( function( require ) {
model.timerVisibleProperty,
tandem.createTandem( 'timerNode' )
);
//REVIEW: JSDoc
this.rulerNode = new DraggableRulerNode(
this.modelViewTransform,
this.visibleBoundsProperty.get(),
@@ -116,13 +122,15 @@ define( function( require ) {
);

// Create specific layer for tools so they don't overlap the reset all button.
//REVIEW: JSDoc
this.toolsLayer = new Node( {
children: [ this.timerNode, this.rulerNode ],
tandem: tandem.createTandem( 'massLayer' ),
preventFit: true
} );

// Reset All button
//REVIEW: JSDoc
this.resetAllButton = new ResetAllButton( {
listener: function() {
model.reset();
@@ -136,24 +144,30 @@ define( function( require ) {
} );

// Sim speed controls
//REVIEW: JSDoc
this.timeControlPanel = new TimeControlNode(
model,
this.visibleBoundsProperty.get(),
tandem.createTandem( 'timeControlPanel' ), {
//REVIEW: Another 0.02 constant to factor out
bottom: this.modelViewTransform.modelToViewY( MassesAndSpringsConstants.FLOOR_Y + 0.02 )
}
);

// sound toggle button at bottom right
//REVIEW: This is never added. Can it just be removed? Why do we have an isSoundEnabledProperty?
var soundToggleButton = new SoundToggleButton( model.isSoundEnabledProperty, {
centerY: this.resetAllButton.centerY - 55,
scale: 0.9
} );
// this.addChild( soundToggleButton );

// Toolbox Panel
//REVIEW: JSDoc
this.toolboxPanel = new ToolboxPanel(
this.visibleBoundsProperty.get(),
//REVIEW: Usually we wouldn't need to pass in the rulerNode/TimerNode (and we wouldn't need to set the toolbox property on the nodes below).
//REVIEW: We can collaborate to remove this dependency?
this.rulerNode, this.timerNode,
model.rulerVisibleProperty,
model.timerVisibleProperty,
@@ -165,9 +179,13 @@ define( function( require ) {

// Adding tools in toolbox
this.addChild( this.toolboxPanel );
//REVIEW: This can be removed presumably if the above is done?
this.timerNode.toolbox = this.toolboxPanel;
this.rulerNode.toolbox = this.toolboxPanel;

//REVIEW: It seems like there are listeners similar to this on subtypes. It's better to create a layout() method
//REVIEW: on the screenview and override/extend as necessary, as you are unintentionally depending on this listener
//REVIEW: being added before the other visibleBoundsProperty listeners are added (for layout)
this.visibleBoundsProperty.link( function( visibleBounds ) {

//Update the bounds of view elements
@@ -189,6 +207,8 @@ define( function( require ) {
massNode.moveToFront();
} );
},

//REVIEW: This can be removed?
//
// /**
// * Responsible for making sure the resetAllButton is in front of the scene graph at all times.
@@ -218,7 +238,7 @@ define( function( require ) {
* Creates a panel that controls the designated spring's spring constant value.
*
* @param {number} springIndex
* @param {array.<Text>} labels
* @param {Array.<Text>} labels
* @param {Tandem} tandem
* @returns {SpringControlPanel}
*/
4 changes: 4 additions & 0 deletions js/common/view/ToolboxPanel.js
Original file line number Diff line number Diff line change
@@ -100,6 +100,8 @@ define( function( require ) {
var rulerUnboundedPosition = new Vector2();

// Drag listener for event forwarding: rulerIcon ---> rulerNode
//REVIEW: Forwarding should generally use SimpleDragHandler.createForwardingListener. I'll be available to help with it.
//REVIEW: Definitely don't need a full listener here, particularly a drag listener.
self.rulerIcon.addInputListener( new MovableDragHandler( rulerNode.positionProperty, {

// allow moving a finger (on a touchscreen) dragged across this node to interact with it
@@ -140,6 +142,8 @@ define( function( require ) {
} );

// Drag listener for event forwarding: timerIcon ---> timerNode
//REVIEW: Forwarding should generally use SimpleDragHandler.createForwardingListener. I'll be available to help with it.
//REVIEW: Definitely don't need a full listener here, particularly a drag listener.
self.timerIcon.addInputListener( new MovableDragHandler( timerNode.positionProperty, {

// allow moving a finger (on a touchscreen) dragged across this node to interact with it
3 changes: 2 additions & 1 deletion js/common/view/TwoSpringScreenView.js
Original file line number Diff line number Diff line change
@@ -36,7 +36,8 @@ define( function( require ) {
* @constructor
*/
function TwoSpringScreenView( model, tandem ) {
this.model = model; // Make model available
//REVIEW: JSDoc
this.model = model; // Make model available REVIEW: This is already done in the supertype, not necessary here
SpringScreenView.call( this, model, tandem );
var self = this;

8 changes: 5 additions & 3 deletions js/lab/view/PeriodTraceNode.js
Original file line number Diff line number Diff line change
@@ -22,22 +22,23 @@ define( function( require ) {

/**
* @constructor
* REVIEW: params
*/
function PeriodTraceNode( periodTrace, modelViewTransform ) {
Node.call( this );
var self = this;

// @protected
// @protected PREVIEW: protected? what inherits from this?
this.periodTrace = periodTrace;

// @private {number} - The opacity of the trace (not using Node opacity for performance reasons)
this.colorAlpha = 1;

// @protected
// @protected PREVIEW: protected? what inherits from this?
this.traceColor = new Color( 'black' );


// @protected
// @protected PREVIEW: protected? what inherits from this?
this.modelViewTransform = modelViewTransform;

this.originalX = this.modelViewTransform.modelToViewX( this.periodTrace.springProperty.value.positionProperty.value.x - 0.2 );
@@ -72,6 +73,7 @@ define( function( require ) {
},

// TODO:documentation
//REVIEW: Looks private. Also since it's just called from step, it COULD be inlined. At least remove the passing in of the MVT and use this.modelViewTransform? (Same for the spring?)
updateTrace: function( spring, modelViewTransform ) {

var mass = spring.massAttachedProperty.value;

0 comments on commit 7eb4b9f

Please sign in to comment.