review items for atomic screenview, see #63

phetsims · Sep 21, 2018 · 6dd8819 · 6dd8819
1 parent a9c10e4
commit 6dd8819
Showing 1 changed file with 16 additions and 37 deletions.
diff --git a/js/atomic/view/CoulombsLawAtomicView.js b/js/atomic/view/CoulombsLawAtomicView.js
@@ -52,12 +52,12 @@ define( function( require ) {
     CoulombsLawCommonView.call( this, coulombsLawModel, SCALE_FACTOR, unitsAtomicUnitsString, MODEL_VIEW_TRANSFORM_SCALE, rulerOptions, tandem );
 
     // charge nodes added in each screen to allow for different decimal precision and arrow height
-    var chargeNode1 = new ChargeNode( 
-      coulombsLawModel, 
-      coulombsLawModel.object1, 
-      this.layoutBounds, 
+    var chargeNode1 = new ChargeNode(
+      coulombsLawModel,
+      coulombsLawModel.object1,
+      this.layoutBounds,
       this.modelViewTransform,
-      tandem.createTandem( 'chargeNode1' ), 
+      tandem.createTandem( 'chargeNode1' ),
       {
         label: charge1AbbreviatedString,
         otherObjectLabel: charge2AbbreviatedString,
@@ -70,12 +70,12 @@ define( function( require ) {
         forceReadoutDecimalPlaces: 9
       } );
 
-    var chargeNode2 = new ChargeNode( 
-      coulombsLawModel, 
-      coulombsLawModel.object2, 
-      this.layoutBounds, 
+    var chargeNode2 = new ChargeNode(
+      coulombsLawModel,
+      coulombsLawModel.object2,
+      this.layoutBounds,
       this.modelViewTransform,
-      tandem.createTandem( 'chargeNode2' ), 
+      tandem.createTandem( 'chargeNode2' ),
       {
         label: charge2AbbreviatedString,
         otherObjectLabel: charge1AbbreviatedString,
@@ -98,58 +98,37 @@ define( function( require ) {
     this.insertChild( 1, chargeNode1.arrowNode );
     this.insertChild( 1, chargeNode2.arrowNode );
 
-    // REVIEW: Remove unused code?
-    // @public (read-only) - create and add atomic ruler
-    // this.coulombsLawRuler = new ISLCRulerNode(
-    //   coulombsLawModel,
-    //   this.layoutBounds.height,
-    //   this.modelViewTransform,
-    //   tandem.createTandem( 'coulombsLawRuler' ),
-    //   {
-    //     snapToNearest: 1E-12, // in model coordinates
-    //     majorTickLabels: [ '0', '10', '20', '30', '40', '50', '60', '70', '80', '90', '100' ],
-    //     unitString: unitsPicometersString,
-    //     rulerInset: 15
-    //   }
-    // );
-    // this.addChild( this.coulombsLawRuler );
-
     // create a line the length of 1 picometer
     var legendNodeLineLength = this.modelViewTransform.modelToViewDeltaX( 10E-12 );
 
-    var legendNode = new ISLCLegendNode( 
+    var legendNode = new ISLCLegendNode(
       legendNodeLineLength, // length of the line
       unitsAtomicLegendScaleString, // unit string
       {
         fill: CoulombsLawColorProfile.legendNodeFillProperty,
         bottom: this.layoutBounds.maxY - 10,
-        tandem: tandem.createTandem( 'atomicLegendNode' ) 
+        left: this.layoutBounds.minX + 9.35,
+        tandem: tandem.createTandem( 'atomicLegendNode' )
       } );
 
-    // REVIEW: Can we move this into the options at the initial declaration?
-    legendNode.left = this.layoutBounds.minX + 9.35;
-
     this.addChild( legendNode );
 
     // add picometer conversion string
-    var pmScaleFont = new PhetFont( 12 );
     var picometerScaleNode = new RichText( pmScaleString, {
       fill: CoulombsLawColorProfile.legendNodeFillProperty,
-      font: pmScaleFont, // REVIEW: Inline pmScaleFont in font option. Removes variable declaration.
+      font: new PhetFont( 12 ),
       maxWidth: 180,
       bottom: this.layoutBounds.maxY - 8,
+      left: legendNode.right + 10,
       tandem: tandem.createTandem( 'picometerScaleString' )
     } );
 
-    // REVIEW: Can we move this into the options at the initial declaration?
-    picometerScaleNode.left = legendNode.right + 10;
-
     this.addChild( picometerScaleNode );
 
     // a11y - charges are first in focus order
     var charges = [ chargeNode1, chargeNode2 ];
 
-    // REVIEW: Visibility annotation? Doc type?
+    // a11y - tab order for the screenview (Accessibility.js setter)
     this.accessibleOrder = charges.concat( this.accessibleOrder );
   }