Skip to content

Commit

Permalink
addressed several REVIEW comments, see #88
Browse files Browse the repository at this point in the history
  • Loading branch information
jbphet committed Jun 5, 2017
1 parent 83a8f77 commit 8151f0d
Show file tree
Hide file tree
Showing 16 changed files with 55 additions and 62 deletions.
2 changes: 1 addition & 1 deletion js/common/model/CoinTerm.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ define( function( require ) {
* @param {string} termText - textual representation, e.g. 'x', must be compatible with SubSupText
* @param {Property.<string>} termValueTextProperty
* @param {CoinTermTypeID} typeID - type identifier for this coin term
* @param {Object} options
* @param {Object} [options]
* @constructor
*/
function CoinTerm( valueProperty, coinRadius, termText, termValueTextProperty, typeID, options ) {
Expand Down
2 changes: 1 addition & 1 deletion js/common/model/CoinTermFactory.js
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ define( function( require ) {
/**
* create a coin term of the specified type
* @param {CoinTermTypeID} typeID
* @param {Object} options - see CoinTerm constructor
* @param {Object} [options] - see CoinTerm constructor
* @returns {CoinTerm}
* @public
*/
Expand Down
2 changes: 1 addition & 1 deletion js/common/view/AbstractCoinTermNode.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ define( function( require ) {

/**
* @param {CoinTerm} coinTerm - model of a coin term
* @param {Object} options
* @param {Object} [options]
* @constructor
*/
function AbstractCoinTermNode( coinTerm, options ) {
Expand Down
2 changes: 1 addition & 1 deletion js/common/view/CoinNodeFactory.js
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ define( function( require ) {
* @returns {Node}
* @param {CoinTermTypeID} coinTermTypeID
* @param {number} radius
* @param {Object} options
* @param {Object} [options]
*/
createIconNode: function( coinTermTypeID, radius, options ) {

Expand Down
2 changes: 1 addition & 1 deletion js/common/view/CoinTermCreatorBox.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ define( function( require ) {

/**
* @param {Array.<CoinTermCreatorNode>} creatorNodes - set of coin term creator nodes
* @param {Object} options
* @param {Object} [options]
* @constructor
*/
function CoinTermCreatorBox( creatorNodes, options ) {
Expand Down
4 changes: 2 additions & 2 deletions js/common/view/CoinTermCreatorBoxFactory.js
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ define( function( require ) {
*
* @param {Object} creatorSetID
* @param {ExpressionManipulationModel} model
* @param {Object} options
* @param {Object} [options]
* @returns {CoinTermCreatorBox}
* @public
*/
Expand Down Expand Up @@ -171,7 +171,7 @@ define( function( require ) {
* @param {ChallengeDescriptor} challengeDescriptor
* @param {number} challengeNumber
* @param {ExpressionManipulationModel} model
* @param {Object} options
* @param {Object} [options]
* @returns {CoinTermCreatorBox}
* @public
*/
Expand Down
2 changes: 1 addition & 1 deletion js/common/view/CoinTermCreatorNode.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ define( function( require ) {
* @param {function} coinTermCreatorFunction - function( {CoinTermTypeID}, options ) : {CoinTerm} - creates the coin
* term model elements that are added to the model, also used for creating "dummy" instances to associate with the
* view nodes that collectively comprise the constructed creator node
* @param {Object} options
* @param {Object} [options]
* @constructor
*/
function CoinTermCreatorNode( expressionManipulationModel,
Expand Down
2 changes: 1 addition & 1 deletion js/common/view/CollectionDisplayNode.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ define( function( require ) {
/**
* @param {ExpressionManipulationModel} model
* @param {Array.<CoinTermTypeID>} displayList - a list of the coin terms types to display in the desired order
* @param {Object} options
* @param {Object} [options]
* @constructor
*/
function CollectionDisplayNode( model, displayList, options ) {
Expand Down
13 changes: 3 additions & 10 deletions js/common/view/ConstantCoinTermNode.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ define( function( require ) {
/**
* @param {CoinTerm} constantCoinTerm - model of a coin
* @param {Property.<ViewMode>} viewModeProperty - controls whether to show the coin or the term
* @param {Object} options
* @param {Object} [options]
* @constructor
*/
function ConstantCoinTermNode( constantCoinTerm, viewModeProperty, options ) {
Expand Down Expand Up @@ -91,16 +91,9 @@ define( function( require ) {
valueText.y = AbstractCoinTermNode.TEXT_BASELINE_Y_OFFSET * constantCoinTerm.scaleProperty.get();

// update the card background
self.cardLikeBackground.visible = false; // make sure card is invisible so it doesn't affect visible bounds
self.cardLikeBackground.setRectBounds( self.coinAndTextRootNode.visibleLocalBounds.dilated( 10 ) );
if ( constantCoinTerm.cardOpacityProperty.get() === 0 ) {
//REVIEW: This was just set to false above?
self.cardLikeBackground.visible = false;
}
else {
self.cardLikeBackground.visible = true;
self.cardLikeBackground.opacity = constantCoinTerm.cardOpacityProperty.get();
}
self.cardLikeBackground.visible = constantCoinTerm.cardOpacityProperty.get() > 0;
self.cardLikeBackground.opacity = constantCoinTerm.cardOpacityProperty.get();

// update the bounds that are registered with the model
updateBoundsInModel();
Expand Down
7 changes: 3 additions & 4 deletions js/common/view/ExpressionManipulationView.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ define( function( require ) {
/**
* @param {ExpressionManipulationModel} model
* @param {Property.<Bounds2>} visibleBoundsProperty
* @param {Object} options
* @param {Object} [options]
* @constructor
*/
function ExpressionManipulationView( model, visibleBoundsProperty, options ) {
Expand Down Expand Up @@ -77,8 +77,7 @@ define( function( require ) {

// add the node that will act as the barrier to interaction with other expressions when editing an expression
var barrierRectangleBounds = null;
var barrierRectangleShape = new Shape(); //REVIEW: This assigned value is never used, as the shape value is re-set?
var barrierRectanglePath = new Path( barrierRectangleShape, {
var barrierRectanglePath = new Path( null, {
fill: 'rgba( 100, 100, 100, 0.5 )',
visible: false, // initially invisible, will become visible when editing an expression
cursor: 'pointer'
Expand All @@ -103,7 +102,7 @@ define( function( require ) {

// define a function that will update the shape of the barrier rectangle
function updateBarrierRectangle() {
barrierRectangleShape = Shape.bounds( barrierRectangleBounds );
var barrierRectangleShape = Shape.bounds( barrierRectangleBounds );
if ( model.expressionBeingEditedProperty.get() ) {
var barrierRectangleHoleBounds = model.expressionBeingEditedProperty.get().getBounds();
// note - must travel counterclockwise to create a hole
Expand Down
4 changes: 3 additions & 1 deletion js/common/view/LeftRightNumberSpinner.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,10 @@ define( function( require ) {
var DEFAULT_MAX_VALUE = 10;

/**
* @param {Propergy.<number>} variableValueProperty - property that wraps the values that will be manipulated
* @param {string} variableString - the variable text displayed in the control
* @param {Object} [options]
* @constructor
* REVIEW: docs?
*/
function LeftRightNumberSpinner( variableValueProperty, variableString, options ) {
Node.call( this );
Expand Down
67 changes: 33 additions & 34 deletions js/common/view/ShowSubtractionIcon.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,52 +31,51 @@ define( function( require ) {
Node.call( this );

// add a rectangle with the first portion of the text
var firstTextPortion = new Text( '+ -x', { font: MATH_FONT } );
var firstTextBackground = new Rectangle(
0,
0,
firstTextPortion.width * 1.4,
firstTextPortion.height * 1.05,
//REVIEW: recommend { cornerRadius: RECT_CORNER_RADIUS }
RECT_CORNER_RADIUS,
RECT_CORNER_RADIUS,
{ fill: RECTANGLE_BACKGROUND_COLOR }
);
//REVIEW: Is this equivalent to firstTextPortion.center = firstTextBackground.center?
firstTextPortion.centerX = firstTextBackground.width / 2;
firstTextPortion.centerY = firstTextBackground.height / 2;
firstTextBackground.addChild( firstTextPortion );
this.addChild( firstTextBackground );
var firstTextWithBackground = new TextWidthBackground( '+ -x' );
this.addChild( firstTextWithBackground );

// add the arrow
var arrow = new ArrowNode( 0, 0, 15, 0, {
left: firstTextBackground.right + 5,
centerY: firstTextBackground.height / 2,
left: firstTextWithBackground.right + 5,
centerY: firstTextWithBackground.height / 2,
stroke: null,
fill: 'rgb( 150, 0, 0 )',
tailWidth: 3,
headHeight: 7
} );
this.addChild( arrow );

//REVIEW: This second* looks somewhat copied from above. Refactor so it's one code path?
// add a rectangle containing the 2nd text portion
var secondTextPortion = new Text( '\u2212 x', { font: MATH_FONT } );
var secondTextBackground = new Rectangle(
0,
0,
secondTextPortion.width * 1.4,
secondTextPortion.height * 1.1,
RECT_CORNER_RADIUS,
RECT_CORNER_RADIUS,
{ fill: RECTANGLE_BACKGROUND_COLOR, left: arrow.right + 5 }
);
secondTextPortion.centerX = secondTextBackground.width / 2;
secondTextPortion.centerY = secondTextBackground.height / 2;
secondTextBackground.addChild( secondTextPortion );
this.addChild( secondTextBackground );
// add the 2nd enclosed text portion
this.addChild( new TextWidthBackground( '\u2212 x', { left: arrow.right + 5 } ) );
}

/**
* inner class for the background box used for the textual portions of the icon
* @param {string} text
* @param {Object} [options]
* @constructor
*/
function TextWidthBackground( text, options ) {

// create the textual node
var textNode = new Text( text, { font: MATH_FONT } );

// create the background, which is a rounded rectangle (the width and height multipliers were empirically determined)
Rectangle.call( this, 0, 0, textNode.width * 1.4, textNode.height * 1.1, {
fill: RECTANGLE_BACKGROUND_COLOR,
cornerRadius: RECT_CORNER_RADIUS
} );

// position and add the text node
textNode.center = this.center;
this.addChild( textNode );

// pass through any options to the parent type
this.mutate( options );
}

inherit( Rectangle, TextWidthBackground );

expressionExchange.register( 'ShowSubtractionIcon', ShowSubtractionIcon );

return inherit( Node, ShowSubtractionIcon );
Expand Down
2 changes: 1 addition & 1 deletion js/common/view/VariableCoinTermNode.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ define( function( require ) {
* @param {Property.<boolean>} showCoinValuesProperty - controls whether or not coin value is shown
* @param {Property.<boolean>} showVariableValuesProperty - controls whether or not variable values are shown
* @param {Property.<boolean>} showAllCoefficientsProperty - controls whether 1 is shown for non-combined coins
* @param {Object} options
* @param {Object} [options]
* @constructor
*/
function VariableCoinTermNode( coinTerm,
Expand Down
2 changes: 1 addition & 1 deletion js/common/view/VariableValueControl.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ define( function( require ) {
var VBox = require( 'SCENERY/nodes/VBox' );

/**
* @param {Object} options
* @param {Object} [options]
* @constructor
*/
function VariableValueControl( options ) {
Expand Down
2 changes: 1 addition & 1 deletion js/game/view/ExpressionDescriptionNode.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ define( function( require ) {
/**
* @param {ExpressionDescription} expressionDescription
* @param {ViewMode} viewMode
* @param {Object} options
* @param {Object} [options]
* @constructor
*/
function ExpressionDescriptionNode( expressionDescription, viewMode, options ) {
Expand Down
2 changes: 1 addition & 1 deletion js/game/view/UndoButton.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ define( function( require ) {

/**
* @constructor
* @param {Object} options
* @param {Object} [options]
*/
function UndoButton( options ) {

Expand Down

0 comments on commit 8151f0d

Please sign in to comment.