Skip to content

Commit

Permalink
Handling more REVIEW statements. phetsims/masses-and-springs#359
Browse files Browse the repository at this point in the history
  • Loading branch information
Denz1994 committed May 11, 2020
1 parent af148e3 commit 7eaceaf
Show file tree
Hide file tree
Showing 8 changed files with 25 additions and 54 deletions.
4 changes: 1 addition & 3 deletions build-a-molecule_en.html
Original file line number Diff line number Diff line change
Expand Up @@ -119,9 +119,7 @@
if ( brand === 'phet-io' ) {
preloads = preloads.concat( [
'../phet-io/js/phet-io-initialize-globals.js',
'../build-a-molecule/js/phet-io/build-a-molecule-baseline.js',
'../build-a-molecule/js/phet-io/build-a-molecule-overrides.js',
'../build-a-molecule/js/phet-io/build-a-molecule-types.js'
'../build-a-molecule/js/phet-io/build-a-molecule-overrides.js'
] );
}

Expand Down
9 changes: 1 addition & 8 deletions js/common/view/KitNode.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
import Shape from '../../../../kite/js/Shape.js';
import BucketFront from '../../../../scenery-phet/js/bucket/BucketFront.js';
import BucketHole from '../../../../scenery-phet/js/bucket/BucketHole.js';
import PhetFont from '../../../../scenery-phet/js/PhetFont.js';
import DragListener from '../../../../scenery/js/listeners/DragListener.js';
import Node from '../../../../scenery/js/nodes/Node.js';
import buildAMolecule from '../../buildAMolecule.js';
Expand Down Expand Up @@ -49,13 +48,7 @@ class KitNode extends Node {
// Create a bucket based on a the kit's model bucket. This includes a front and back for the bucket contained in
// different layout.
kit.buckets.forEach( bucket => {
const bucketFront = new BucketFront( bucket, BAMConstants.MODEL_VIEW_TRANSFORM, {
//REVIEW: I can't see where this labelFont option is used? Can you find the API point?
labelFont: new PhetFont( {
weight: 'bold',
size: 18
} )
} );
const bucketFront = new BucketFront( bucket, BAMConstants.MODEL_VIEW_TRANSFORM );
const bucketHole = new BucketHole( bucket, BAMConstants.MODEL_VIEW_TRANSFORM );
// NOTE: we will use the Bucket's hole with an expanded touch area to trigger the "grab by touching the bucket" behavior
bucketHole.touchArea = bucketHole.mouseArea = new Shape()
Expand Down
2 changes: 0 additions & 2 deletions js/common/view/KitPanel.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,6 @@ class KitPanel extends Node {
this.kitCarousel = new Carousel( kitNodes, {
fill: BAMConstants.KIT_BACKGROUND,
stroke: BAMConstants.KIT_BORDER,
//REVIEW: I don't see xMargin in any doc for Carousel, what is the option doing here?
xMargin: 10,
itemsPerPage: 1,
animationEnabled: false,
buttonSoundPlayer: Playable.NO_SOUND
Expand Down
31 changes: 14 additions & 17 deletions js/common/view/MoleculeBondNode.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,22 +11,19 @@ import platform from '../../../../phet-core/js/platform.js';
import Circle from '../../../../scenery/js/nodes/Circle.js';
import Line from '../../../../scenery/js/nodes/Line.js';
import Node from '../../../../scenery/js/nodes/Node.js';
import closedUpCursorImage from '../../../images/scissors-closed-up_cur.js';
import closedUpImage from '../../../images/scissors-closed-up_png.js';
import closedCursorImage from '../../../images/scissors-closed_cur.js';
import closedImage from '../../../images/scissors-closed_png.js';
import upCursorImage from '../../../images/scissors-up_cur.js';
import upImage from '../../../images/scissors-up_png.js';
import cursorImage from '../../../images/scissors_cur.js';
import scissorsClosedUpCursorImage from '../../../images/scissors-closed-up_cur.js';
import scissorsClosedUpImage from '../../../images/scissors-closed-up_png.js';
import scissorsClosedCursorImage from '../../../images/scissors-closed_cur.js';
import scissorsClosedImage from '../../../images/scissors-closed_png.js';
import scissorsUpCursorImage from '../../../images/scissors-up_cur.js';
import scissorsUpImage from '../../../images/scissors-up_png.js';
import scissorsCursorImage from '../../../images/scissors_cur.js';
import scissorsImage from '../../../images/scissors_png.js';
import buildAMolecule from '../../buildAMolecule.js';
import BAMConstants from '../BAMConstants.js';
import Direction from '../model/Direction.js';
import FireListener from '../../../../scenery/js/listeners/FireListener.js';

//REVIEW: Imports don't match up with the image files, I would expect to see:
//REVIEW: scissorsClosedUpImage, scissorsClosedUpImage, scissorsClosedImage, scissorsClosedImage, scissorsUpImage, scissorsUpImage, scissorsImage

/* Notes on .cur file generation, all from the images directory, with "sudo apt-get install icoutils" for icotool:
icotool -c -o scissors.ico scissors.png
icotool -c -o scissors-closed.ico scissors-closed.png
Expand All @@ -41,13 +38,13 @@ import FireListener from '../../../../scenery/js/listeners/FireListener.js';

const images = {
'scissors.png': scissorsImage,
'scissors-closed.png': closedImage,
'scissors-up.png': upImage,
'scissors-closed-up.png': closedUpImage,
'scissors.cur': cursorImage,
'scissors-closed.cur': closedCursorImage,
'scissors-up.cur': upCursorImage,
'scissors-closed-up.cur': closedUpCursorImage
'scissors-closed.png': scissorsClosedImage,
'scissors-up.png': scissorsUpImage,
'scissors-closed-up.png': scissorsClosedUpImage,
'scissors.cur': scissorsCursorImage,
'scissors-closed.cur': scissorsClosedCursorImage,
'scissors-up.cur': scissorsUpCursorImage,
'scissors-closed-up.cur': scissorsClosedUpCursorImage
};
const bondRadius = 6; // "Radius" of the bond target that will break the bond

Expand Down
3 changes: 0 additions & 3 deletions js/common/view/WarningDialog.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,6 @@ class WarningDialog extends Dialog {
],
spacing: 15,
align: 'center',

//REVIEW: What is this xMargin option for?
xMargin: 15,
cursor: 'pointer'
} );
warningNode.mouseArea = warningNode.touchArea = warningNode.localBounds;
Expand Down
10 changes: 3 additions & 7 deletions js/multiple/MultipleScreen.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,9 @@ class MultipleScreen extends BAMScreen {
backgroundColorProperty: new Property( BAMConstants.PLAY_AREA_BACKGROUND_COLOR ),
homeScreenIcon: BAMIconFactory.createMultipleScreen()
};
super( //REVIEW: Prefer non-block arrow functions here, e.g. () => new MultipleModel(),...
() => {
return new MultipleModel();
},
model => {
return new MoleculeCollectingScreenView( model, false );
},
super(
() => new MultipleModel(),
model => new MoleculeCollectingScreenView( model, false ),
options );
}
}
Expand Down
10 changes: 3 additions & 7 deletions js/playground/PlaygroundScreen.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,9 @@ class PlaygroundScreen extends BAMScreen {
backgroundColorProperty: new Property( BAMConstants.PLAY_AREA_BACKGROUND_COLOR ),
homeScreenIcon: BAMIconFactory.createPlaygroundScreen()
};
super( //REVIEW: Prefer non-block arrow functions here, e.g. () => new PlaygroundModel(),...
() => {
return new PlaygroundModel();
},
model => {
return new BAMScreenView( model );
},
super(
() => new PlaygroundModel(),
model => new BAMScreenView( model ),
options
);
}
Expand Down
10 changes: 3 additions & 7 deletions js/single/SingleScreen.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,9 @@ class SingleScreen extends BAMScreen {
backgroundColorProperty: new Property( BAMConstants.PLAY_AREA_BACKGROUND_COLOR ),
homeScreenIcon: BAMIconFactory.createSingleScreen()
};
super( //REVIEW: Prefer non-block arrow functions here, e.g. () => new SingleModel(),...
() => {
return new SingleModel();
},
model => {
return new MoleculeCollectingScreenView( model, true );
},
super(
() => new SingleModel(),
model => new MoleculeCollectingScreenView( model, true ),
options
);
}
Expand Down

0 comments on commit 7eaceaf

Please sign in to comment.