REVIEW notes, see phetsims/my-solar-system#88

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
   },

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(
       {

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<boolean>,
                                        wasDraggedProperty: TReadOnlyProperty<boolean> ): TReadOnlyProperty<boolean> {
     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,

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<Vector2>;
 
   protected readonly modelViewTransformProperty: ReadOnlyProperty<ModelViewTransform2>;
@@ -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,