REVIEW comments, #32

js/common-sm/view/CustomLauncherSection.ts

@@ -1,5 +1,6 @@
 // Copyright 2023-2024, University of Colorado Boulder
 
+//REVIEW Used only in Sources screen. Why is this in common-sm/?
 /**
  * This CustomLauncherSection is a panel section allows the user to customize a launcher.
  *

js/common-vsm/view/AngleStabilizerSection.ts

@@ -1,5 +1,7 @@
 // Copyright 2023-2024, University of Colorado Boulder
 
+//REVIEW Doc says 'A specific NumberControl' but I see no use of NumberControl herein.
+//REVIEW Used only in Sources screen, why is this in common-vsm/?
 /**
  * A specific NumberControl for this sim that changes the angle standard deviation for the launcher. Note that in the UI
  * it is named "Angle Stabilizer" to convey the mechanism, but throughout the code the actual value that is being set

js/common-vsm/view/HeatMapToolNode.ts

@@ -107,6 +107,9 @@ type SelfOptions = {
 export type HeatMapToolNodeOptions = SelfOptions & NodeOptions;
 
 export default class HeatMapToolNode extends Node {
+
+  //REVIEW Lots of fields here that could use documentation.
+
   private readonly minValue: number;
   private readonly maxValue: number;
   private readonly binWidth: number;
@@ -264,7 +267,7 @@ export default class HeatMapToolNode extends Node {
       heatNodes.push( heatNode );
 
       // Initialize the number of values in each bin to 0
-      this.numValuesInBin.push( 0 );
+      this.numValuesInBin.push( 0 ); //REVIEW This is a side effect that is not obvious. Are you sure you want to do this here? Maybe do this after calling createHeatNodes.
     }
 
     return heatNodes;

js/common-vsm/view/InteractiveToolPanel.ts

@@ -85,19 +85,25 @@ export default class InteractiveToolPanel extends PDLPanel {
     const checkboxGroupTandem = options.tandem.createTandem( 'checkboxGroup' );
     const stopwatchCheckboxTandemName = 'stopwatchCheckbox';
 
-    const checkboxGroup = new VerticalCheckboxGroup( [ {
-      property: isMeasuringTapeVisibleProperty,
-      createNode: () => new PDLCheckboxRow( ProjectileDataLabStrings.measuringTapeStringProperty, new MeasuringTapeIconNode() ),
-      tandemName: 'measuringTapeCheckbox'
-    }, {
-      property: isStopwatchVisibleProperty,
-      createNode: () => new PDLCheckboxRow( ProjectileDataLabStrings.stopwatchStringProperty, new StopwatchNodeIcon() ),
-      tandemName: stopwatchCheckboxTandemName,
-      options: {
-        visibleProperty: createGatedVisibleProperty( DerivedProperty.not( PDLPreferences.autoGenerateDataProperty ),
-          checkboxGroupTandem.createTandem( stopwatchCheckboxTandemName ) )
-      }
-    },
+    const checkboxGroup = new VerticalCheckboxGroup( [
+
+      // Measuring tape
+      {
+        property: isMeasuringTapeVisibleProperty,
+        createNode: () => new PDLCheckboxRow( ProjectileDataLabStrings.measuringTapeStringProperty, new MeasuringTapeIconNode() ),
+        tandemName: 'measuringTapeCheckbox'
+      },
+
+      // Stopwatch
+      {
+        property: isStopwatchVisibleProperty,
+        createNode: () => new PDLCheckboxRow( ProjectileDataLabStrings.stopwatchStringProperty, new StopwatchNodeIcon() ),
+        tandemName: stopwatchCheckboxTandemName,
+        options: {
+          visibleProperty: createGatedVisibleProperty( DerivedProperty.not( PDLPreferences.autoGenerateDataProperty ),
+            checkboxGroupTandem.createTandem( stopwatchCheckboxTandemName ) )
+        }
+      },
       ...options.additionalVerticalCheckboxGroupItems
     ], {
       tandem: checkboxGroupTandem,

js/common-vsm/view/ProjectileSelectorNode.ts

@@ -87,6 +87,7 @@ export default class ProjectileSelectorNode extends SelectorNode {
       landedImageIndex?: number;
     };
 
+    //REVIEW Does each instance need its own depictions, or can these be shared via 'const DEPICTIONS' outside the class definition?
     const depictions: Depiction[] = [
       { type: CANNONBALL, isFlippedHorizontally: false },
       { type: CANNONBALL, isFlippedHorizontally: true },
@@ -100,6 +101,7 @@ export default class ProjectileSelectorNode extends SelectorNode {
       { type: PUMPKIN, isFlippedHorizontally: true, landedImageIndex: 2 }
     ];
 
+    //REVIEW My first thought was 'Does this need to be stateful?' If not, document why not.
     const depictionProperty = new Property<Depiction>( depictions[ 0 ] );
 
     const createNode = ( depiction: Depiction ) => {

js/common-vsm/view/SpeedToolNode.ts

@@ -22,6 +22,8 @@ import TReadOnlyProperty from '../../../../axon/js/TReadOnlyProperty.js';
 type SelfOptions = {
   isIcon?: boolean;
 };
+
+//REVIEW So complicated! So much simpler: SelfOptions & NodeTranslationOptions & PickOptional<HeatMapToolNodeOptions, 'visibleProperty'>
 export type SpeedToolNodeOptions = SelfOptions & StrictOmit<HeatMapToolNodeOptions,
   'displayOffset' | 'readoutPatternStringProperty' | 'bodyShape' | 'needleShape' | 'binWidth' | 'minValue'
   | 'maxValue' | 'minLabeledValue' | 'maxLabeledValue' | 'labeledValueIncrement' | 'labelDistanceFromCenter' | 'labelMinAngle'

js/common-vsm/view/VSMFieldSignNode.ts

@@ -1,5 +1,6 @@
 // Copyright 2023-2024, University of Colorado Boulder
 
+//REVIEW Incomplete doc. What does this class add to FieldSignNode?
 /**
  * The VSMFieldSignNode shows the field number and the number of projectiles that have landed in that field.
  *