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
jbphet committed Jun 7, 2017
1 parent 9c44334 commit 5ceaf56
Show file tree
Hide file tree
Showing 5 changed files with 91 additions and 155 deletions.
43 changes: 11 additions & 32 deletions js/game/view/EECollectionAreaNode.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,38 +25,22 @@ define( function( require ) {
Node.call( this );

// create the 'halo' that will turn on as a hint that the user can drop something into the collection area
//REVIEW: Rectangle.bounds( collectionArea.bounds, { cornerRadius: CORNER_RADIUS, ... } ) if we don't translate by the bounds minX/minY
var halo = new Rectangle(
0,
0,
collectionArea.bounds.width,
collectionArea.bounds.height,
CORNER_RADIUS,
CORNER_RADIUS,
{
lineWidth: 9,
stroke: '#66FF33'
}
);
var halo = Rectangle.bounds( collectionArea.bounds, {
lineWidth: 9,
stroke: '#66FF33',
cornerRadius: CORNER_RADIUS
} );
this.addChild( halo );

// control halo visibility
collectionArea.haloActiveProperty.linkAttribute( halo, 'visible' );

// create the basic rectangular background
//REVIEW: Rectangle.bounds( collectionArea.bounds, { cornerRadius: CORNER_RADIUS, ... } ) if we don't translate by the bounds minX/minY
var collectionAreaRectangle = new Rectangle(
0,
0,
collectionArea.bounds.width,
collectionArea.bounds.height,
CORNER_RADIUS,
CORNER_RADIUS,
{
fill: 'white',
stroke: 'black'
}
);
var collectionAreaRectangle = Rectangle.bounds( collectionArea.bounds, {
fill: 'white',
stroke: 'black',
cornerRadius: CORNER_RADIUS
} );
this.addChild( collectionAreaRectangle );

// add the expression description representation, which will update if the expression description changes
Expand All @@ -66,13 +50,11 @@ define( function( require ) {
// remove the previous expression description node, if present
if ( expressionDescriptionNode ) {
self.removeChild( expressionDescriptionNode );
//REVIEW: Usually with this pattern, the expressionDescriptionNode would be set to null? Otherwise if the
// description goes to null and back, things error?
expressionDescriptionNode = null;
}

// add the description node for the new expression
if ( expressionDescription ) {
//REVIEW: If we don't translate by the bounds minX/minY (as suggested above), just use things relative to the bounds instead
expressionDescriptionNode = new ExpressionDescriptionNode(
expressionDescription,
collectionArea.viewMode,
Expand All @@ -81,9 +63,6 @@ define( function( require ) {
self.addChild( expressionDescriptionNode );
}
} );

//REVIEW: Recommend removing this, as it makes the Rectangle constructors a lot more verbose (and is an extra line)
this.setTranslation( collectionArea.bounds.minX, collectionArea.bounds.minY );
}

expressionExchange.register( 'EECollectionAreaNode', EECollectionAreaNode );
Expand Down
55 changes: 24 additions & 31 deletions js/game/view/EEGameIconNode.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,9 @@ define( function( require ) {
var EESharedConstants = require( 'EXPRESSION_EXCHANGE/common/EESharedConstants' );
var expressionExchange = require( 'EXPRESSION_EXCHANGE/expressionExchange' );
var FaceNode = require( 'SCENERY_PHET/FaceNode' );
var HBox = require( 'SCENERY/nodes/HBox' );
var HStrut = require( 'SCENERY/nodes/HStrut' );
var inherit = require( 'PHET_CORE/inherit' );
var Node = require( 'SCENERY/nodes/Node' );
var PhetFont = require( 'SCENERY_PHET/PhetFont' );
var Rectangle = require( 'SCENERY/nodes/Rectangle' );
var Screen = require( 'JOIST/Screen' );
Expand All @@ -26,77 +27,69 @@ define( function( require ) {
var ICON_SIZE = Screen.MINIMUM_HOME_SCREEN_ICON_SIZE;
var COIN_SPACING = ICON_SIZE.width * 0.02; // empirically determined
var TEXT_FONT = new PhetFont( 50 );
var PLUS_SIGN_X_MARGIN = 10;

/**
* @constructor
*/
function EEGameIconNode() {

// create the background
//REVIEW: Rectangle.dimension( ICON_SIZE, { fill: BACKGROUND_COLOR } )
Rectangle.call( this, 0, 0, ICON_SIZE.width, ICON_SIZE.height, { fill: BACKGROUND_COLOR } );

// created a rounded rectangle that looks like a card
//REVIEW: Recommend cornerRadius (duplicated) specified once in options object
var cardBackground = new Rectangle( 0, 0, ICON_SIZE.width / 2, ICON_SIZE.height / 2, 20, 20, {
var cardBackground = new Rectangle( 0, 0, ICON_SIZE.width / 2, ICON_SIZE.height / 2, {
x: ICON_SIZE.width * 0.1,
y: ICON_SIZE.height * 0.1,
fill: 'white',
stroke: 'black',
lineWidth: 2
lineWidth: 2,
cornerRadius: 20
} );

this.addChild( cardBackground );

// create a "coin equation" that includes coins and numbers
var coinEquation = new Node();
coinEquation.addChild( new Text( '2', { font: TEXT_FONT } ) );
var coinRadius = ICON_SIZE.width * 0.04; // empirically determined
//REVIEW: Can an HBox be used here? looks like anti-pattern with incrementing left based on extisting width.
coinEquation.addChild( CoinNodeFactory.createIconNode( CoinTermTypeID.X, coinRadius, {
left: coinEquation.width,
centerY: coinEquation.centerY
} ) );
coinEquation.addChild( new Text( '+', {
font: TEXT_FONT,
left: coinEquation.width + 10
} ) );
coinEquation.addChild( new Text( '3', {
font: TEXT_FONT,
left: coinEquation.width + 10
} ) );
coinEquation.addChild( CoinNodeFactory.createIconNode( CoinTermTypeID.Y, coinRadius, {
left: coinEquation.width,
centerY: coinEquation.centerY
} ) );
var coinEquation = new HBox( {
children: [
new Text( '2', { font: TEXT_FONT } ),
CoinNodeFactory.createIconNode( CoinTermTypeID.X, coinRadius ),
new HStrut( PLUS_SIGN_X_MARGIN ),
new Text( '+', { font: TEXT_FONT } ),
new HStrut( PLUS_SIGN_X_MARGIN ),
new Text( '3', { font: TEXT_FONT } ),
CoinNodeFactory.createIconNode( CoinTermTypeID.Y, coinRadius )
]
} );

// add the coin equation to the card
//REVIEW: coinEquation.center = cardBackground.localBounds.center?
coinEquation.centerX = cardBackground.width / 2;
coinEquation.centerY = cardBackground.height / 2;
cardBackground.addChild( coinEquation );

// create and add coins next to the card
//REVIEW: Has some values that could be factored out (and named for improved readability)
var topCoinRowCenterY = cardBackground.top + cardBackground.height * 0.3;
var secondCoinRowCenterY = cardBackground.top + cardBackground.height * 0.7;
this.addChild( CoinNodeFactory.createIconNode( CoinTermTypeID.X, coinRadius, {
left: cardBackground.right + COIN_SPACING,
centerY: cardBackground.top + cardBackground.height * 0.3
centerY: topCoinRowCenterY
} ) );
this.addChild( CoinNodeFactory.createIconNode( CoinTermTypeID.X, coinRadius, {
left: cardBackground.right + coinRadius * 2 + COIN_SPACING * 2,
centerY: cardBackground.top + cardBackground.height * 0.3
centerY: topCoinRowCenterY
} ) );
this.addChild( CoinNodeFactory.createIconNode( CoinTermTypeID.Y, coinRadius, {
left: cardBackground.right + COIN_SPACING,
centerY: cardBackground.top + cardBackground.height * 0.7
centerY: secondCoinRowCenterY
} ) );
this.addChild( CoinNodeFactory.createIconNode( CoinTermTypeID.Y, coinRadius, {
left: cardBackground.right + coinRadius * 2 + COIN_SPACING * 2,
centerY: cardBackground.top + cardBackground.height * 0.7
centerY: secondCoinRowCenterY
} ) );
this.addChild( CoinNodeFactory.createIconNode( CoinTermTypeID.Y, coinRadius, {
left: cardBackground.right + coinRadius * 4 + COIN_SPACING * 3,
centerY: cardBackground.top + cardBackground.height * 0.7
centerY: secondCoinRowCenterY
} ) );

this.addChild( new FaceNode( ICON_SIZE.width * 0.35, {
Expand Down
71 changes: 22 additions & 49 deletions js/game/view/EEGameLevelIconFactory.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,73 +8,50 @@ define( function( require ) {
'use strict';

// modules
var CoinNodeFactory = require( 'EXPRESSION_EXCHANGE/common/view/CoinNodeFactory' );
var CoinTermTypeID = require( 'EXPRESSION_EXCHANGE/common/enum/CoinTermTypeID' );
var EESharedConstants = require( 'EXPRESSION_EXCHANGE/common/EESharedConstants' );
var expressionExchange = require( 'EXPRESSION_EXCHANGE/expressionExchange' );
var Image = require( 'SCENERY/nodes/Image' );
var Node = require( 'SCENERY/nodes/Node' );
var PhetFont = require( 'SCENERY_PHET/PhetFont' );
var Rectangle = require( 'SCENERY/nodes/Rectangle' );
var Text = require( 'SCENERY/nodes/Text' );

// images
var coinXTimesYFrontImage = require( 'mipmap!EXPRESSION_EXCHANGE/coin-xy-back.png' );
var coinYBackImage = require( 'mipmap!EXPRESSION_EXCHANGE/coin-y-back.png' );
var coinYSquaredBackImage = require( 'mipmap!EXPRESSION_EXCHANGE/coin-y-squared-back.png' );

// constants
var CARD_CORNER_ROUNDING = 4;
var NUMBER_LABEL_FONT = new PhetFont( 26 ); // size empirically determined
var CARD_STAGGER_OFFSET = 1.5; // empirically determined, same in x and y directions
var ICON_WIDTH = 40;
var CARD_ICON_HEIGHT = 40;
var COIN_RADIUS = 20; // empirically determined

// helper function for creating coin-image-based icons
//REVIEW Type docs generally helpful when reading this type of function
/**
* helper function for creating coin-image-based icons
* @param {CoinTermTypeID} coinTermTypeID - type of coin image to use
* @param {number} value - value that will appear on the face of the coin
* @returns {Node}
*/
function createCoinIcon( coinTermTypeID, value ) {

var rootNode = new Node();
var imageNode;

// create the coin image node
//REVIEW: These images are also used in similar switch statements in CoinNodeFactory. This could be potentially
// factored out into CoinNodeFactory?
switch( coinTermTypeID ) {
case CoinTermTypeID.X:
imageNode = new Image( coinXTimesYFrontImage );
break;

case CoinTermTypeID.Y:
imageNode = new Image( coinYBackImage );
break;

case CoinTermTypeID.Z:
imageNode = new Image( coinYSquaredBackImage );
break;

default:
assert && assert( false, 'handling does not exist for coin term type: ' + coinTermTypeID );
break;
}

imageNode.setScaleMagnitude( ICON_WIDTH / imageNode.width );
rootNode.addChild( imageNode );
var imageNode = CoinNodeFactory.createImageNode( coinTermTypeID, COIN_RADIUS, false );

// add the label
rootNode.addChild( new Text( value, {
var label = new Text( value, {
font: NUMBER_LABEL_FONT,
centerX: imageNode.width / 2,
centerY: imageNode.height / 2
} ) );

//REVIEW: Easier to
// return new Node( { children: [ imageNode, the_text_thing ] } ) rather than having an additional declaration and return.
} );

return rootNode;
return new Node( { children: [ imageNode, label ] } );
}

// helper function for creating the icons that look like stacks of cards
//REVIEW Type docs generally helpful when reading this type of function
/**
* helper function for creating the icons that look like a stack of cards
* @param {number} numberOnStack
* @param {number} numberOfAdditionalCards
* @returns {Node}
*/
function createCardStackIcon( numberOnStack, numberOfAdditionalCards ) {
var rootNode = new Node();
var cardWidth = ICON_WIDTH - numberOfAdditionalCards * CARD_STAGGER_OFFSET;
Expand All @@ -100,9 +77,7 @@ define( function( require ) {
} ) );

// add the cards to the root node
//REVIEW: cards.reverse() returns the reversed array, so that can be chained with the forEach directly
cards.reverse();
cards.forEach( function( card ) {
cards.reverse().forEach( function( card ) {
rootNode.addChild( card );
} );

Expand All @@ -125,20 +100,18 @@ define( function( require ) {

var icon;

//REVIEW: If helpful, I've found it nice to have different objects for each level that handle creation of icons.
//REVIEW: No problem to leave as-is if this is the most convenient.
switch( gameLevel ) {

case 0:
icon = createCoinIcon( CoinTermTypeID.X, 1 );
icon = createCoinIcon( CoinTermTypeID.X_TIMES_Y, 1 );
break;

case 1:
icon = createCoinIcon( CoinTermTypeID.Y, 2 );
icon = createCoinIcon( CoinTermTypeID.X, 2 );
break;

case 2:
icon = createCoinIcon( CoinTermTypeID.Z, 3 );
icon = createCoinIcon( CoinTermTypeID.Y_SQUARED, 3 );
break;

case 3:
Expand Down
Loading

0 comments on commit 5ceaf56

Please sign in to comment.