diff --git a/js/game/view/EEGameScreenView.js b/js/game/view/EEGameScreenView.js index e11b772a..a5be3f47 100644 --- a/js/game/view/EEGameScreenView.js +++ b/js/game/view/EEGameScreenView.js @@ -17,7 +17,7 @@ define( function( require ) { var GameAudioPlayer = require( 'VEGAS/GameAudioPlayer' ); var inherit = require( 'PHET_CORE/inherit' ); var ScreenView = require( 'JOIST/ScreenView' ); - var StartGameLevelNode = require( 'EXPRESSION_EXCHANGE/game/view/StartGameLevelNode' ); + var LevelSelectionNode = require( 'EXPRESSION_EXCHANGE/game/view/LevelSelectionNode' ); // constants var SCREEN_CHANGE_TIME = 1000; // milliseconds @@ -50,18 +50,13 @@ define( function( require ) { } ); // add the node that allows the user to choose a game level to play - var levelSelectionNode = new StartGameLevelNode( + var levelSelectionNode = new LevelSelectionNode( function( level ) { gameModel.selectLevel( level ); }, function() { gameModel.reset(); }, gameModel.soundEnabledProperty, levelSelectionButtonIcons, levelScoreProperties, { - numStarsOnButtons: EEGameModel.CHALLENGES_PER_LEVEL, - perfectScore: EEGameModel.MAX_SCORE_PER_LEVEL, - numLevels: EEGameModel.NUMBER_OF_LEVELS, - numButtonRows: 2, - controlsInset: 10, size: this.layoutBounds, centerX: this.layoutBounds.centerX } diff --git a/js/game/view/StartGameLevelNode.js b/js/game/view/LevelSelectionNode.js similarity index 65% rename from js/game/view/StartGameLevelNode.js rename to js/game/view/LevelSelectionNode.js index 573b589c..2c0cb0d8 100644 --- a/js/game/view/StartGameLevelNode.js +++ b/js/game/view/LevelSelectionNode.js @@ -2,10 +2,6 @@ /** * A node that fills most of the screen and allows the user to select the game level that they wish to play. - * REVIEW: Lots of duplicated code here with other files of the same name. Should be consolidated into vegas, or - * simplified in each use case. - * - * REVIEW: Created as a 'levelSelectionNode' variable, can we change name to LevelSelectionNode? * * @author John Blanco */ @@ -14,6 +10,7 @@ define( function( require ) { // modules var expressionExchange = require( 'EXPRESSION_EXCHANGE/expressionExchange' ); + var EEGameModel = require( 'EXPRESSION_EXCHANGE/game/model/EEGameModel' ); var EESharedConstants = require( 'EXPRESSION_EXCHANGE/common/EESharedConstants' ); var inherit = require( 'PHET_CORE/inherit' ); var LevelSelectionButton = require( 'VEGAS/LevelSelectionButton' ); @@ -32,52 +29,50 @@ define( function( require ) { /** * @param {Function} startLevelFunction - Function used to initiate a game level, will be called with a zero-based - * index value. + * index value. * @param {Function} resetFunction - Function to reset game and scores. * @param {Property.} soundEnabledProperty * @param {Array.} iconNodes - Set of iconNodes to use on the buttons, sizes should be the same, length of array - * must match number of levels. + * must match number of levels. * @param {Array.>} scores - Current scores, used to decide which stars to illuminate on the level - * start buttons, length must match number of levels. - * REVIEW: added type doc here, but can you verify it's correct? + * start buttons, length must match number of levels. * @param {Object} [options] - See code below for options and default values. * @constructor */ - function StartGameLevelNode( startLevelFunction, resetFunction, soundEnabledProperty, iconNodes, scores, options ) { + function LevelSelectionNode( startLevelFunction, resetFunction, soundEnabledProperty, iconNodes, scores, options ) { Node.call( this ); - //REVIEW: Options not really ever needed, should just be inlined (there is only one constructor call!) options = _.extend( { // defaults - numLevels: 4, // REVIEW: Move actual value (EEGameModel.NUMBER_OF_LEVELS) from EEGameScreenView to here. + numLevels: EEGameModel.NUMBER_OF_LEVELS, titleString: chooseYourLevelString, maxTitleWidth: 500, - numStarsOnButtons: 5, // REVIEW: Move actual value (EEGameModel.CHALLENGES_PER_LEVEL) from EEGameScreenView to here. - perfectScore: 10, // REVIEW: Move actual value (EEGameModel.MAX_SCORE_PER_LEVEL) from EEGameScreenView to here. + numStarsOnButtons: EEGameModel.CHALLENGES_PER_LEVEL, + perfectScore: EEGameModel.MAX_SCORE_PER_LEVEL, buttonBackgroundColor: '#EDA891', - numButtonRows: 1, // For layout REVIEW: Move actual value (2) from EEGameScreenView to here. Confusing to have two values when only one is ever used - controlsInset: 12, // REVIEW: move from EEGameScreenView, actual value 10 - size: EESharedConstants.LAYOUT_BOUNDS // REVIEW: Don't specify default here, only usage uses layoutBounds + numButtonRows: 2, + controlsInset: 10, + size: EESharedConstants.LAYOUT_BOUNDS, + buttonScale: 0.8 }, options ); // Verify parameters - //REVIEW: Looks like this should be an assertion instead? - if ( iconNodes.length !== options.numLevels || scores.length !== options.numLevels ) { - throw new Error( 'Number of game levels doesn\'t match length of provided arrays' ); - } + assert( + iconNodes.length === options.numLevels && scores.length === options.numLevels, + 'Number of game levels doesn\'t match length of provided arrays' + ); - // Title + // title var title = new Text( options.titleString, { font: new PhetFont( 30 ), maxWidth: options.maxTitleWidth } ); this.addChild( title ); - // Add the buttons + // add the buttons function createLevelStartFunction( level ) { return function() { startLevelFunction( level ); }; } - //REVIEW: Using a game Level object should help clean this code up? var buttons = new Array( options.numLevels ); for ( var i = 0; i < options.numLevels; i++ ) { buttons[ i ] = new LevelSelectionButton( @@ -87,11 +82,10 @@ define( function( require ) { scores[ i ], options.perfectScore, { - baseColor: options.buttonBackgroundColor + baseColor: options.buttonBackgroundColor, + scale: options.buttonScale } ); - //REVIEW: Why is this scale done here, instead of { scale: 0.8 } in the above options? (and 0.8 instead of 0.80) - buttons[ i ].scale( 0.80 ); this.addChild( buttons[ i ] ); } @@ -111,7 +105,6 @@ define( function( require ) { this.addChild( resetButton ); // Layout - //REVIEW: Lots of copy-pasted code from other StartGameLevelNodes? Clean up, use layout boxes if possible var numColumns = options.numLevels / options.numButtonRows; var buttonSpacingX = buttons[ 0 ].width * 1.2; // Note: Assumes all buttons are the same size. var buttonSpacingY = buttons[ 0 ].height * 1.2; // Note: Assumes all buttons are the same size. @@ -132,7 +125,7 @@ define( function( require ) { soundToggleButton.bottom = options.size.height - options.controlsInset; } - expressionExchange.register( 'StartGameLevelNode', StartGameLevelNode ); + expressionExchange.register( 'LevelSelectionNode', LevelSelectionNode ); - return inherit( Node, StartGameLevelNode ); + return inherit( Node, LevelSelectionNode ); } ); diff --git a/js/game/view/NextLevelNode.js b/js/game/view/NextLevelNode.js index 75e5810e..ada5de9d 100644 --- a/js/game/view/NextLevelNode.js +++ b/js/game/view/NextLevelNode.js @@ -30,7 +30,6 @@ define( function( require ) { var faceNode = new FaceNode( FACE_DIAMETER ); this.addChild( faceNode ); - //REVIEW: Replace the layout code with a VBox with spacing 10? var button = new RectangularPushButton( { content: new Text( nextString, { font: new PhetFont( 30 ) } ), centerX: faceNode.centerX, diff --git a/js/game/view/UndoButton.js b/js/game/view/UndoButton.js index 78c75b3e..a45bad1c 100644 --- a/js/game/view/UndoButton.js +++ b/js/game/view/UndoButton.js @@ -34,21 +34,19 @@ define( function( require ) { arrowFill: 'black' }, options ); + assert && assert( !options.content, 'content should not be specified for this button' ); + // create the shape for the undo arrow var undoArrowShape = new Shape() .moveTo( 0, 0 ) .lineTo( 0, ICON_HEIGHT ) .lineTo( ICON_HEIGHT, ICON_HEIGHT ) .lineTo( ICON_HEIGHT * 0.7, ICON_HEIGHT * 0.7 ) - //.lineTo( ICON_HEIGHT * 2, ICON_HEIGHT * 0.5 ) // REVIEW: can these commented out lines be removed or tagged with an issue? .quadraticCurveTo( ICON_HEIGHT * 1.25, -ICON_HEIGHT * 0.1, ICON_HEIGHT * 2, ICON_HEIGHT * 0.75 ) - //.lineTo( ICON_HEIGHT * 0.25, ICON_HEIGHT * 0.25 ) .quadraticCurveTo( ICON_HEIGHT * 1.25, -ICON_HEIGHT * 0.5, ICON_HEIGHT * 0.3, ICON_HEIGHT * 0.3 ) - .lineTo( 0, 0 ) // REVIEW: this was the starting point, so the extra lineTo isn't necessary before the close. .close(); // set up the content node - //REVIEW: CM would prefer an assertion to make sure this wasn't already specified in the options. options.content = new Path( undoArrowShape, { fill: options.arrowFill } ); diff --git a/js/negatives/view/EENegativesIconNode.js b/js/negatives/view/EENegativesIconNode.js index 0e98576e..fcc7dc36 100644 --- a/js/negatives/view/EENegativesIconNode.js +++ b/js/negatives/view/EENegativesIconNode.js @@ -11,6 +11,7 @@ define( function( require ) { // modules var EESharedConstants = require( 'EXPRESSION_EXCHANGE/common/EESharedConstants' ); var expressionExchange = require( 'EXPRESSION_EXCHANGE/expressionExchange' ); + var HBox = require( 'SCENERY/nodes/HBox' ); var inherit = require( 'PHET_CORE/inherit' ); var MathSymbolFont = require( 'SCENERY_PHET/MathSymbolFont' ); var Node = require( 'SCENERY/nodes/Node' ); @@ -36,20 +37,15 @@ define( function( require ) { Rectangle.call( this, 0, 0, ICON_SIZE.width, ICON_SIZE.height, { fill: BACKGROUND_COLOR } ); // create and add the equation node - //REVIEW: Relies on Text.left being 0. Seems like we can just use HBox here to simplify logic? - var equationNode = new Node(); - equationNode.addChild( new Text( '3', { font: TEXT_FONT } ) ); - equationNode.addChild( new RichText( 'x2', { - font: MATH_FONT, - supScale: 0.5, - left: equationNode.width - } ) ); - equationNode.addChild( new Text( ' \u2212 ', { font: TEXT_FONT, left: equationNode.width } ) ); - equationNode.addChild( new RichText( 'x2', { - font: MATH_FONT, - supScale: 0.5, - left: equationNode.width - } ) ); + var equationNode = new HBox( { + children: [ + new Text( '3', { font: TEXT_FONT } ), + new RichText( 'x2', { font: MATH_FONT, supScale: 0.5 } ), + new Text( ' \u2212 ', { font: TEXT_FONT } ), + new RichText( 'x2', { font: MATH_FONT, supScale: 0.5 } ) + ], + align: 'bottom' + } ); equationNode.centerX = ICON_SIZE.width / 2; equationNode.centerY = ICON_SIZE.height * 0.45; this.addChild( equationNode );