Handling review comments, phetsims/my-solar-system#95

phetsims · Mar 15, 2023 · 6a4f000 · 6a4f000
1 parent 5687aa3
commit 6a4f000
Show file tree

Hide file tree

Showing 3 changed files with 19 additions and 26 deletions.
diff --git a/js/SolarSystemCommonConstants.ts b/js/SolarSystemCommonConstants.ts
@@ -53,10 +53,7 @@ const SolarSystemCommonConstants = {
     fill: SolarSystemCommonColors.foregroundProperty
   },
 
-  //REVIEW: Why not GRID_SPACING? I don't see any point of nesting here
-  GRID: {
-    spacing: 100
-  },
+  GRID_SPACING: 100,
 
   NUM_BODIES: 4,
 

diff --git a/js/view/SolarSystemCommonScreenView.ts b/js/view/SolarSystemCommonScreenView.ts
@@ -59,7 +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?
+  // Used only on Keplers Laws but is defined here because of the Model View Transform
   protected readonly orbitalCenterProperty: Property<Vector2>;
 
   protected readonly modelViewTransformProperty: ReadOnlyProperty<ModelViewTransform2>;
@@ -112,15 +112,15 @@ class SolarSystemCommonScreenView extends ScreenView {
         return ModelViewTransform2.createSinglePointScaleInvertedYMapping(
           Vector2.ZERO,
           new Vector2(
-            orbitalCenter.x - SolarSystemCommonConstants.GRID.spacing,
-            orbitalCenter.y - SolarSystemCommonConstants.GRID.spacing ),
+            orbitalCenter.x - SolarSystemCommonConstants.GRID_SPACING,
+            orbitalCenter.y - SolarSystemCommonConstants.GRID_SPACING ),
           zoom );
       } );
 
     // Add the node for the overlay grid, setting its visibility based on the model.showGridProperty
     this.interfaceLayer.addChild( new GridNode(
       this.modelViewTransformProperty,
-      SolarSystemCommonConstants.GRID.spacing,
+      SolarSystemCommonConstants.GRID_SPACING,
       Vector2.ZERO,
       28,
       {
@@ -188,7 +188,7 @@ class SolarSystemCommonScreenView extends ScreenView {
         } ),
         new TextPushButton( SolarSystemCommonStrings.clearStringProperty, {
           font: new PhetFont( 16 ),
-          listener: () => { model.timeProperty.reset(); }, //REVIEW: don't need the braces around this
+          listener: () => model.timeProperty.reset(),
           maxTextWidth: 65,
           tandem: providedOptions.tandem.createTandem( 'clearButton' ),
           touchAreaXDilation: 10,

diff --git a/js/view/SolarSystemCommonTextNumberDisplay.ts b/js/view/SolarSystemCommonTextNumberDisplay.ts
@@ -16,30 +16,26 @@ import SolarSystemCommonStrings from '../../../solar-system-common/js/SolarSyste
 import StringUtils from '../../../phetcommon/js/util/StringUtils.js';
 import solarSystemCommon from '../solarSystemCommon.js';
 
-//REVIEW: Single usage, prefer inlining (then don't have to define the type, AND won't need the extra empty {} object
-//REVIEW: allocation that is in the optionize3 call below). Can just use optionize
-const STRING_PATTERN_OPTIONS: NumberDisplayOptions = {
-  backgroundFill: null,
-  backgroundStroke: null,
-  textOptions: SolarSystemCommonConstants.TEXT_OPTIONS,
-  decimalPlaces: 1,
-  useRichText: true,
-
-  //REVIEW: layoutOptions like this should be provided at the client usage. It's not great to hardcode client uses
-  //REVIEW: in the type itself. Let's move this to the client!
-  layoutOptions: {
-    align: 'left'
-  }
-};
-
 export default class SolarSystemCommonTextNumberDisplay extends NumberDisplay {
   //REVIEW: Ummm... I don't see this constructor ever getting used. Can we remove it, and not extend NumberDisplay?
   //REVIEW: Perhaps we can just move combinePowerString to somewhere else common, and remove this file?
   public constructor( numberProperty: TReadOnlyProperty<number | null>, displayRange: Range, providedOptions?: NumberDisplayOptions ) {
     super(
       numberProperty,
       displayRange,
-      optionize3<NumberDisplayOptions, EmptySelfOptions, NumberDisplayOptions>()( {}, STRING_PATTERN_OPTIONS, providedOptions )
+      optionize3<NumberDisplayOptions, EmptySelfOptions, NumberDisplayOptions>()( {}, {
+        backgroundFill: null,
+        backgroundStroke: null,
+        textOptions: SolarSystemCommonConstants.TEXT_OPTIONS,
+        decimalPlaces: 1,
+        useRichText: true,
+
+        //REVIEW: layoutOptions like this should be provided at the client usage. It's not great to hardcode client uses
+        //REVIEW: in the type itself. Let's move this to the client!
+        layoutOptions: {
+          align: 'left'
+        }
+      }, providedOptions )
     );
   }