diff --git a/js/SolarSystemCommonConstants.ts b/js/SolarSystemCommonConstants.ts index 86e36b8..2daddf1 100644 --- a/js/SolarSystemCommonConstants.ts +++ b/js/SolarSystemCommonConstants.ts @@ -15,6 +15,8 @@ const SolarSystemCommonConstants = { MARGIN: 15, SCREEN_VIEW_X_MARGIN: 15, SCREEN_VIEW_Y_MARGIN: 15, + + //REVIEW: maximum width for.... what? Should be documented, and possibly renamed to something more informative MAX_WIDTH: 200, CONTROL_PANEL_OPTIONS: { @@ -50,6 +52,8 @@ const SolarSystemCommonConstants = { font: new PhetFont( { size: 18, weight: 'bold' } ), fill: SolarSystemCommonColors.foregroundProperty }, + + //REVIEW: Why not GRID_SPACING? I don't see any point of nesting here GRID: { spacing: 100 }, diff --git a/js/view/BodyNode.ts b/js/view/BodyNode.ts index 7e91f13..664e47f 100644 --- a/js/view/BodyNode.ts +++ b/js/view/BodyNode.ts @@ -225,6 +225,9 @@ export default class BodyNode extends ShadedSphereNode { this.body.collidedEmitter.addListener( bodyCollisionListener ); + //REVIEW: This createVisibleProperty is only used here, AND we are creating a useless BooleanProperty for it. + //REVIEW: please inline and simplify as a DerivedProperty here, as we only need to base it off of the + //REVIEW: userControlledProperty! const cueingVisibleProperty = CueingArrowsNode.createVisibleProperty( new BooleanProperty( options.draggable ), this.body.userControlledProperty ); const cueingArrowsNode = new CueingArrowsNode( { diff --git a/js/view/CueingArrowsNode.ts b/js/view/CueingArrowsNode.ts index 92169b4..3ad4881 100644 --- a/js/view/CueingArrowsNode.ts +++ b/js/view/CueingArrowsNode.ts @@ -27,6 +27,7 @@ type CueingArrowsNodeOptions = SelfOptions & export default class CueingArrowsNode extends Path { // length of the arrows, from tip to tip + //REVIEW: We don't use this field anywhere but the constructor. Get rid of this field (won't even need a local variable) private readonly length: number; private readonly disposeCueingArrowsNode: () => void; @@ -55,8 +56,11 @@ export default class CueingArrowsNode extends Path { this.touchArea = this.localBounds.dilated( 5 ); this.mouseArea = this.localBounds.dilated( 3 ); }; + //REVIEW: This depends on the localBounds, so it should link to the localBoundsProperty instead this.boundsProperty.link( updateTouchArea ); + //REVIEW: Don't need/want a disposal of the updateTouchArea, since they have the same lifetime + //REVIEW: I'd just inline the updateTouchArea function into the link this.disposeCueingArrowsNode = () => { this.boundsProperty.unlink( updateTouchArea ); }; @@ -67,6 +71,8 @@ export default class CueingArrowsNode extends Path { this.disposeCueingArrowsNode(); } + //REVIEW: Only one usage. See notes at the usage for inlining and simplifying. + //REVIEW: I don't see a point of having this function here, and it's a ton of TypeScript verbiage. public static createVisibleProperty( inputEnabledProperty: TReadOnlyProperty, wasDraggedProperty: TReadOnlyProperty ): TReadOnlyProperty { return new DerivedProperty( @@ -76,6 +82,8 @@ export default class CueingArrowsNode extends Path { } } +//REVIEW: This looks somewhat copied from CueingArrowsNode in geometric-optics. Can we factor out cue arrow shape +//REVIEW: creation to somewhere in common code, instead of copying? const ARROW_SHAPE_OPTIONS = { doubleHead: false, headWidth: 12, diff --git a/js/view/SolarSystemCommonScreenView.ts b/js/view/SolarSystemCommonScreenView.ts index 250bcfe..f7b3214 100644 --- a/js/view/SolarSystemCommonScreenView.ts +++ b/js/view/SolarSystemCommonScreenView.ts @@ -59,6 +59,7 @@ class SolarSystemCommonScreenView extends ScreenView { protected readonly createDraggableVectorNode: ( body: Body, options?: DraggableVectorNodeOptions ) => DraggableVectorNode; // View position of where the geometrical center of the orbit is located + //REVIEW: Somewhat surprised this isn't in the model, but I'm ok with it here. Were both locations considered? protected readonly orbitalCenterProperty: Property; protected readonly modelViewTransformProperty: ReadOnlyProperty; @@ -186,7 +187,7 @@ class SolarSystemCommonScreenView extends ScreenView { } ), new TextPushButton( SolarSystemCommonStrings.clearStringProperty, { font: new PhetFont( 16 ), - listener: () => { model.timeProperty.reset(); }, + listener: () => { model.timeProperty.reset(); }, //REVIEW: don't need the braces around this maxTextWidth: 65, tandem: providedOptions.tandem.createTandem( 'clearButton' ), touchAreaXDilation: 10,