From 518ec1a50432badabfb26a3fa822d71f8ac40cd2 Mon Sep 17 00:00:00 2001 From: Chris Malley Date: Tue, 18 Dec 2018 16:15:42 -0700 Subject: [PATCH] first pass through coding convention in view, #259 Signed-off-by: Chris Malley --- js/common/WaveInterferenceConstants.js | 5 +++++ js/common/WaveInterferenceUtils.js | 4 +++- js/common/model/Scene.js | 4 ++-- js/common/model/ViewType.js | 1 + js/common/model/WaterScene.js | 4 ++-- js/common/model/WaveTemporalType.js | 1 + js/common/view/DashedLineNode.js | 1 + js/common/view/EmitterNode.js | 1 + js/common/view/InputTypeIconNode.js | 6 +++++- js/common/view/IntensityGraphPanel.js | 2 +- js/common/view/LatticeCanvasNode.js | 1 + js/common/view/LengthScaleIndicatorNode.js | 2 +- js/common/view/LightEmitterNode.js | 1 + js/common/view/LightScreenNode.js | 4 +++- js/common/view/Perspective3DNode.js | 5 +++-- js/common/view/PulseContinuousRadioButtonGroup.js | 1 + js/common/view/SoundEmitterNode.js | 1 + js/common/view/SoundParticleLayer.js | 4 ++++ js/common/view/TimeControls.js | 2 +- js/common/view/ToolboxPanel.js | 6 ++++-- js/common/view/ViewRadioButtonGroup.js | 1 + js/common/view/WaterDropImage.js | 1 + js/common/view/WaterDropLayer.js | 1 + js/common/view/WaterEmitterNode.js | 5 +++-- js/common/view/WaterSideViewNode.js | 1 + js/common/view/WaveAreaGraphNode.js | 2 ++ js/common/view/WaveInterferenceControlPanel.js | 7 +++++++ js/common/view/WaveInterferenceSceneIcons.js | 1 + js/common/view/WaveInterferenceSlider.js | 2 +- js/common/view/WaveInterferenceText.js | 2 ++ js/common/view/WaveMeterProbeNode.js | 3 +++ js/interference/view/InterferenceScreenView.js | 3 +++ js/slits/view/BarriersNode.js | 5 ++++- js/slits/view/PlaneWaveGeneratorNode.js | 1 + js/slits/view/SlitsControlPanel.js | 2 ++ js/slits/view/TheoryInterferenceOverlay.js | 2 +- js/wave-interference-main.js | 1 + js/waves/view/WavesScreenView.js | 1 + 38 files changed, 78 insertions(+), 19 deletions(-) diff --git a/js/common/WaveInterferenceConstants.js b/js/common/WaveInterferenceConstants.js index c0e42f7e..6200dd8c 100644 --- a/js/common/WaveInterferenceConstants.js +++ b/js/common/WaveInterferenceConstants.js @@ -55,12 +55,14 @@ define( require => { CHART_LINE_JOIN: 'round', // Look of the emitter button across all 3 scenes + //REVIEW where is the color for the green emitter button in pulse mode? EMITTER_BUTTON_COLOR: 'red', EMITTER_BUTTON_RADIUS: 14, EMITTER_BUTTON_TOUCH_AREA_DILATION: 8, FEMTO: 1E-15, + //REVIEW no idea how I'd coordinate this, and WaterEmitterNode waterDrops does not exist // Cell that oscillates, specified as an offset from the origin of the lattice (includes damping region). This // value must be coordinated with WaterEmitterNode's waterDrops[ index ].centerX POINT_SOURCE_HORIZONTAL_COORDINATE: 23, @@ -79,11 +81,14 @@ define( require => { // maxWidth for slider ticks TICK_MAX_WIDTH: 30, + //REVIEW 'a smidge smaller than the reset of the texts' means a smidge smaller than DEFAULT_FONT, yes? Then factor out DEFAULT_FONT_SIZE and make it so. // Use for the time and length scale texts above the wave area, looks best to be a smidge smaller than the // rest of the texts TIME_AND_LENGTH_SCALE_INDICATOR_FONT: new PhetFont( 14 ) }; + //REVIEW assert that WaveInterferenceConstants.LATTICE_DIMENSION is an odd integer + waveInterference.register( 'WaveInterferenceConstants', WaveInterferenceConstants ); return WaveInterferenceConstants; diff --git a/js/common/WaveInterferenceUtils.js b/js/common/WaveInterferenceUtils.js index c1e3d3e5..2e5b09e8 100644 --- a/js/common/WaveInterferenceUtils.js +++ b/js/common/WaveInterferenceUtils.js @@ -23,7 +23,6 @@ define( require => { class WaveInterferenceUtils { - /** * Gets a Shape representing the top of the water in water side view from left to right, also used for the chart. * @param {number[]} array - reused to avoid allocations @@ -60,6 +59,8 @@ define( require => { * @public */ static getWaterSideY( waveAreaBounds, waveValue ) { + + //REVIEW explain the magic numbers here return Util.linear( 0, 5, waveAreaBounds.centerY, waveAreaBounds.centerY - 80, waveValue ); } @@ -97,6 +98,7 @@ define( require => { return value / WaveInterferenceConstants.FEMTO; } + //REVIEW unnecessary coupling, passing in the entire model when only model.waterScene.lattice.visibleBounds is needed /** * Gets the horizontal coordinate where water drops come out--aligned with the oscillation cell. * @param {WavesScreenModel} model diff --git a/js/common/model/Scene.js b/js/common/model/Scene.js index 21d5baef..686b9343 100644 --- a/js/common/model/Scene.js +++ b/js/common/model/Scene.js @@ -62,11 +62,11 @@ define( require => { // @private {number} - indicates the time when the pulse began, or 0 if there is no pulse. this.pulseStartTime = 0; - // @public {Property.} - whether the button for the first source is pressed. This is also used for the + // @public whether the button for the first source is pressed. This is also used for the // slits screen plane wave source. this.button1PressedProperty = new BooleanProperty( false ); - // @public {Property.} - whether the button for the second source is pressed + // @public whether the button for the second source is pressed this.button2PressedProperty = new BooleanProperty( false ); // @public (read-only) {string} - units for this scene diff --git a/js/common/model/ViewType.js b/js/common/model/ViewType.js index 93d6d8fe..7de0ccbf 100644 --- a/js/common/model/ViewType.js +++ b/js/common/model/ViewType.js @@ -1,5 +1,6 @@ // Copyright 2018, University of Colorado Boulder +//REVIEW ViewType is a pretty weak name. ViewpointEnum? /** * Selects whether the wave is shown from the top (default) or side view. This value represents the user selection-- * the view animates between the selections. diff --git a/js/common/model/WaterScene.js b/js/common/model/WaterScene.js index 69f30dac..fb8c024a 100644 --- a/js/common/model/WaterScene.js +++ b/js/common/model/WaterScene.js @@ -67,8 +67,8 @@ define( require => { /** * Fire another water drop if one is warranted for the specified faucet. We already determined that the timing * is right (for the period), now we need to know if a drop should fire. - * @param {Property.} buttonProperty - indicates whether the corresponding button is pressed - * @param {Property.} oscillatingProperty - indicates whether the wave source is oscillating + * @param {BooleanProperty} buttonProperty - indicates whether the corresponding button is pressed + * @param {BooleanProperty} oscillatingProperty - indicates whether the wave source is oscillating * @param {number} sign - -1 for top faucet, +1 for bottom faucet * @private */ diff --git a/js/common/model/WaveTemporalType.js b/js/common/model/WaveTemporalType.js index 646af91a..4025134c 100644 --- a/js/common/model/WaveTemporalType.js +++ b/js/common/model/WaveTemporalType.js @@ -1,5 +1,6 @@ // Copyright 2018, University of Colorado Boulder +//REVIEW a better name for this might be DisturbanceType. A wave is a continuous disturbance. A pulse is a one-time disturbance. /** * Determines whether the incoming wave is continuous or pulse. * diff --git a/js/common/view/DashedLineNode.js b/js/common/view/DashedLineNode.js index 2dc2755b..303d206f 100644 --- a/js/common/view/DashedLineNode.js +++ b/js/common/view/DashedLineNode.js @@ -1,5 +1,6 @@ // Copyright 2018, University of Colorado Boulder +//REVIEW This doc was confusing. It's not a 'dotted line', it's a dashed line with stroked dashes. /** * When the graph is selected, the dotted line is shown in the center of the WaveAreaNode and in the graph's center. * diff --git a/js/common/view/EmitterNode.js b/js/common/view/EmitterNode.js index 9389c583..fa376952 100644 --- a/js/common/view/EmitterNode.js +++ b/js/common/view/EmitterNode.js @@ -1,5 +1,6 @@ // Copyright 2018, University of Colorado Boulder +//REVIEW #272 Clarify 'emitter' in this context, or use a synonym, to prevent confusion with AXON/Emitter. /** * For each scene, shows one node for each emitter, each with its own on/off button. * diff --git a/js/common/view/InputTypeIconNode.js b/js/common/view/InputTypeIconNode.js index 0040d508..d872cb26 100644 --- a/js/common/view/InputTypeIconNode.js +++ b/js/common/view/InputTypeIconNode.js @@ -1,5 +1,6 @@ // Copyright 2018, University of Colorado Boulder +//REVIEW This is a visual representation of WaveTemporalType, so why is this class name totally different? why not WaveTermTypeNode? /** * Shows the icons for the radio buttons that choose between pulse and continuous waves. * @@ -26,11 +27,13 @@ define( require => { class InputTypeIconNode extends Node { /** - * @param {WaveTemporalType} incidentWaveType + * @param {WaveTemporalType} incidentWaveType //REVIEW rename to waveTemporalType * @param {Object} [options] */ constructor( incidentWaveType, options ) { super(); + + //REVIEW Implementation is unnecessarily complicated because you're trying to do both icons in one class. Separate into 2 classes? const minAngle = incidentWaveType === WaveTemporalType.PULSE ? Math.PI : 0; const minX = incidentWaveType === WaveTemporalType.PULSE ? MARGIN : 0; const maxX = incidentWaveType === WaveTemporalType.PULSE ? ( WIDTH - MARGIN ) : WIDTH; @@ -56,6 +59,7 @@ define( require => { if ( incidentWaveType === WaveTemporalType.PULSE ) { shape.lineToRelative( MARGIN, 0 ); } + this.addChild( new Path( shape, { stroke: 'black', lineWidth: 2 diff --git a/js/common/view/IntensityGraphPanel.js b/js/common/view/IntensityGraphPanel.js index 4b788d89..c2f95e56 100644 --- a/js/common/view/IntensityGraphPanel.js +++ b/js/common/view/IntensityGraphPanel.js @@ -52,7 +52,7 @@ define( require => { } ); /** - * Creates a line an the given y-coordinate. + * Creates a line on the given y-coordinate. * @param {number} index * @param {number} y * @returns {Line} diff --git a/js/common/view/LatticeCanvasNode.js b/js/common/view/LatticeCanvasNode.js index a4212dcc..4ad5a091 100644 --- a/js/common/view/LatticeCanvasNode.js +++ b/js/common/view/LatticeCanvasNode.js @@ -95,6 +95,7 @@ define( require => { * Draws into the canvas. * @param {CanvasRenderingContext2D} context * @public + * @override */ paintCanvas( context ) { diff --git a/js/common/view/LengthScaleIndicatorNode.js b/js/common/view/LengthScaleIndicatorNode.js index bca64ac1..a73cccdf 100644 --- a/js/common/view/LengthScaleIndicatorNode.js +++ b/js/common/view/LengthScaleIndicatorNode.js @@ -2,7 +2,7 @@ /** * Appears above the lattice and shows the scale, like this: - * |<------>| 500 nanometers + * |<------>| 500 nm * * @author Sam Reid (PhET Interactive Simulations) */ diff --git a/js/common/view/LightEmitterNode.js b/js/common/view/LightEmitterNode.js index 90937447..55f12d06 100644 --- a/js/common/view/LightEmitterNode.js +++ b/js/common/view/LightEmitterNode.js @@ -1,5 +1,6 @@ // Copyright 2018, University of Colorado Boulder +//REVIEW #272 clarify emitter in this context /** * For the light scene, shows one laser pointer for each emitter, each with its own on/off button. * diff --git a/js/common/view/LightScreenNode.js b/js/common/view/LightScreenNode.js index 837ad2c5..aaf154d8 100644 --- a/js/common/view/LightScreenNode.js +++ b/js/common/view/LightScreenNode.js @@ -52,7 +52,7 @@ define( require => { // @private this.intensitySample = intensitySample; - // @private + // @private {Color} required because we'll be operating on a Color this.baseColor = new Color( 'blue' ); // Render into a sub-canvas which will be drawn into the rendering context at the right scale. @@ -96,6 +96,8 @@ define( require => { /** * Draws into the canvas. * @param {CanvasRenderingContext2D} context + * @public + * @override */ paintCanvas( context ) { diff --git a/js/common/view/Perspective3DNode.js b/js/common/view/Perspective3DNode.js index 0ebab915..bc112c67 100644 --- a/js/common/view/Perspective3DNode.js +++ b/js/common/view/Perspective3DNode.js @@ -28,7 +28,7 @@ define( require => { /** * @param {Bounds2} waveAreaBounds * @param {NumberProperty} rotationAmountProperty - * @param {DerivedProperty.} isRotatingProperty + * @param {DerivedProperty.} isRotatingProperty //REVIEW why does this need to be a DerivedProperty? */ constructor( waveAreaBounds, rotationAmountProperty, isRotatingProperty ) { @@ -108,7 +108,8 @@ define( require => { } /** - * @private - update the shapes and text when the rotationAmount has changed + * Update the shapes and text when the rotationAmount has changed + * @private */ update() { diff --git a/js/common/view/PulseContinuousRadioButtonGroup.js b/js/common/view/PulseContinuousRadioButtonGroup.js index ab4c846e..e5640f10 100644 --- a/js/common/view/PulseContinuousRadioButtonGroup.js +++ b/js/common/view/PulseContinuousRadioButtonGroup.js @@ -1,5 +1,6 @@ // Copyright 2018, University of Colorado Boulder +//REVIEW since this is related to WaveTemporalType, rename to WaveTemporalTypeRadioButtonGroup ? /** * Shows the "pulse" vs "continuous" radio buttons. * diff --git a/js/common/view/SoundEmitterNode.js b/js/common/view/SoundEmitterNode.js index 0e066919..6a2a3dec 100644 --- a/js/common/view/SoundEmitterNode.js +++ b/js/common/view/SoundEmitterNode.js @@ -1,5 +1,6 @@ // Copyright 2018, University of Colorado Boulder +//REVIEW #272 clarify emitter in this context /** * For the sound scene, shows one speaker for each emitter, each with its own on/off button. * diff --git a/js/common/view/SoundParticleLayer.js b/js/common/view/SoundParticleLayer.js index 9f09ec4f..6eca08f0 100644 --- a/js/common/view/SoundParticleLayer.js +++ b/js/common/view/SoundParticleLayer.js @@ -15,6 +15,7 @@ define( require => { const waveInterference = require( 'WAVE_INTERFERENCE/waveInterference' ); // constants + //REVIEW say more about RESOLUTION. Looks like same semantics as resolution option to Node.rasterized, but I had to work for it. const RESOLUTION = 2; // Render at increased resolution so particles don't appear pixellated on a large screen. class SoundParticleLayer extends CanvasNode { @@ -45,6 +46,7 @@ define( require => { waveAreaNodeBounds ); + //REVIEW looks like this could be factored out of constructor const toImage = color => new ShadedSphereNode( 10, { mainColor: color, stroke: 'black' @@ -72,6 +74,8 @@ define( require => { /** * Draws into the canvas. * @param {CanvasRenderingContext2D} context + * @public + * @override */ paintCanvas( context ) { context.transform( 1 / RESOLUTION, 0, 0, 1 / RESOLUTION, 0, 0 ); diff --git a/js/common/view/TimeControls.js b/js/common/view/TimeControls.js index 55740bae..190ed982 100644 --- a/js/common/view/TimeControls.js +++ b/js/common/view/TimeControls.js @@ -1,7 +1,7 @@ // Copyright 2018, University of Colorado Boulder /** - * Buttons for play/pause radio buttons for normal/slow + * Buttons for play/pause, and radio buttons for normal/slow * * @author Sam Reid (PhET Interactive Simulations) */ diff --git a/js/common/view/ToolboxPanel.js b/js/common/view/ToolboxPanel.js index 34496ab0..df05947b 100644 --- a/js/common/view/ToolboxPanel.js +++ b/js/common/view/ToolboxPanel.js @@ -64,7 +64,7 @@ define( require => { // Make sure the probes have enough breathing room so they don't get shoved into the WaveMeterNode icon // The true value is set when dragging - waveMeterNode.backgroundNode.translate( 100, 0 ); + waveMeterNode.backgroundNode.translate( 100, 0 ); //REVIEW magic number? // The draggable icon, which has an overlay to make the buttons draggable instead of pressable // Temporarily show the node so it can be rasterized for an icon @@ -92,6 +92,8 @@ define( require => { waveMeterIcon ] } ) ), { + + // Panel options yMargin: 9.55, maxWidth: WaveInterferenceConstants.PANEL_MAX_WIDTH } @@ -102,7 +104,7 @@ define( require => { /** * Initialize the icon for use in the toolbox. * @param {Node} node - * @param {Property.} inPlayAreaProperty + * @param {Property.} inPlayAreaProperty * @param {Object} forwardingListener */ const initializeIcon = ( node, inPlayAreaProperty, forwardingListener ) => { diff --git a/js/common/view/ViewRadioButtonGroup.js b/js/common/view/ViewRadioButtonGroup.js index 2cc7508b..f5eb2a3a 100644 --- a/js/common/view/ViewRadioButtonGroup.js +++ b/js/common/view/ViewRadioButtonGroup.js @@ -1,5 +1,6 @@ // Copyright 2018, University of Colorado Boulder +//REVIEW since this is related to ViewType, rename to ViewTypeRadioButtonGroup ? /** * Selects between Top View and Side View. * diff --git a/js/common/view/WaterDropImage.js b/js/common/view/WaterDropImage.js index 11d69fd5..33d9498e 100644 --- a/js/common/view/WaterDropImage.js +++ b/js/common/view/WaterDropImage.js @@ -24,6 +24,7 @@ define( require => { constructor( options ) { super( waterDropImage, options ); + //REVIEW {WaterDrop|null} and presumably null means no associated WaterDrop instance? // @public {WaterDrop} - Link each Image to its corresponding WaterDrop, so that when the view goes underwater, // we can mark the corresponding model as absorbed. this.waterDrop = null; diff --git a/js/common/view/WaterDropLayer.js b/js/common/view/WaterDropLayer.js index 3ce18964..c601d67f 100644 --- a/js/common/view/WaterDropLayer.js +++ b/js/common/view/WaterDropLayer.js @@ -37,6 +37,7 @@ define( require => { const MAX_DROPS = 4; const waterDropNodes = _.times( MAX_DROPS, () => new WaterDropImage() ); + //REVIEW assert( !options || !options.children ) or your children may immediately be replaced by this.mutate this.children = waterDropNodes; this.mutate( options ); diff --git a/js/common/view/WaterEmitterNode.js b/js/common/view/WaterEmitterNode.js index a438daaa..1622a558 100644 --- a/js/common/view/WaterEmitterNode.js +++ b/js/common/view/WaterEmitterNode.js @@ -1,5 +1,6 @@ // Copyright 2018, University of Colorado Boulder +//REVIEW #272 clarify emitter in this context /** * For the water scene, shows one hose for each emitter, each with its own on/off button. This implementation is trivial * and doesn't add state or methods, it simplifies readability at the call site, so we keep it as a convenience @@ -33,8 +34,8 @@ define( require => { constructor( model, waveAreaNode, isPrimarySource ) { const faucetNode = new FaucetNode( - // This value is irrelevant because we use our own faucet water emitting model, but must be nonzero - // to prevent a divide by zero problem + // This value for maxFlowRate is irrelevant because we use our own faucet water emitting model, + // but must be nonzero to prevent a divide by zero problem 1, // Flow rate is managed by this simulation and not depicted by the FaucetNode diff --git a/js/common/view/WaterSideViewNode.js b/js/common/view/WaterSideViewNode.js index 1b6725c0..fbda9f48 100644 --- a/js/common/view/WaterSideViewNode.js +++ b/js/common/view/WaterSideViewNode.js @@ -27,6 +27,7 @@ define( require => { */ constructor( waveAreaBounds, model ) { + //REVIEW vestigial comment and annotation? // @private - depicts the side face (when the user selects "side view") super( null, { lineJoin: WaveInterferenceConstants.CHART_LINE_JOIN, diff --git a/js/common/view/WaveAreaGraphNode.js b/js/common/view/WaveAreaGraphNode.js index e0daf8ec..7d5a3b97 100644 --- a/js/common/view/WaveAreaGraphNode.js +++ b/js/common/view/WaveAreaGraphNode.js @@ -43,6 +43,7 @@ define( require => { const graphWidth = WaveInterferenceConstants.WAVE_AREA_WIDTH; + //REVIEW nice to know that graphHeight and dy must be synchronized, but how? // Manually tuned so the graph is the appropriate height, must be synchronized with dy below. const graphHeight = WaveInterferenceConstants.WAVE_AREA_WIDTH * 0.3833333333333333; @@ -171,6 +172,7 @@ define( require => { centerY: plotHeight / 2 } ) ); + //REVIEW describe what's going on here [ 1 / 4, 3 / 4 ].forEach( horizontalGridLineFraction => { this.addChild( new Line( 0, horizontalGridLineFraction * plotHeight, diff --git a/js/common/view/WaveInterferenceControlPanel.js b/js/common/view/WaveInterferenceControlPanel.js index c9d8db8c..d0bcec33 100644 --- a/js/common/view/WaveInterferenceControlPanel.js +++ b/js/common/view/WaveInterferenceControlPanel.js @@ -1,5 +1,6 @@ // Copyright 2018, University of Colorado Boulder +//REVIEW There are definitely similarities in this control panel for the 3 scenes. But the reuse of what's common in this implementation feels forced/complicated. I'd hate to have to maintain this. /** * Shows the main controls, including frequency/wavelength and amplitude. * @@ -61,6 +62,7 @@ define( require => { maxWidth: WaveInterferenceConstants.PANEL_MAX_WIDTH }, options ); + //REVIEW 'metric coordinate frame' is not one of the 3 coordinate frames described in implmentation notes. What is this? // Controls are in the metric coordinate frame const waterFrequencySlider = new WaveInterferenceSlider( model.getWaterFrequencySliderProperty(), @@ -111,6 +113,8 @@ define( require => { ); } ); + //REVIEW worthy of factoring out into its own class file, SoundViewTypeRadioButtonGroup? + //REVIEW PhET-iO: rename const to soundViewTypeRadioButtonGroup, since it's setting SoundViewType const viewSelectionRadioButtonGroup = new VerticalAquaRadioButtonGroup( [ { node: new WaveInterferenceText( wavesString ), value: SoundViewType.WAVES, @@ -163,6 +167,8 @@ define( require => { // Create faucet icon, and rasterize to clip out invisible parts (like the ShooterNode) const sceneIcons = new WaveInterferenceSceneIcons(); + //REVIEW worthy of factoring out into its own class file, SceneRadioButtonGroup? + //REVIEW PhET-iO: rename const to sceneRadioButtonGroup const sceneRadioButtons = new RadioButtonGroup( model.sceneProperty, [ { value: model.waterScene, node: sceneIcons.faucetIcon }, { value: model.soundScene, node: sceneIcons.speakerIcon }, @@ -266,6 +272,7 @@ define( require => { ]; } ); + //REVIEW move assignments of point areas closer to definition of these checkboxes? graphCheckbox.mouseArea = graphCheckbox.localBounds.dilated( 2 ).withX( separator.right ); graphCheckbox.touchArea = graphCheckbox.mouseArea; diff --git a/js/common/view/WaveInterferenceSceneIcons.js b/js/common/view/WaveInterferenceSceneIcons.js index 8ab8e2eb..04f97786 100644 --- a/js/common/view/WaveInterferenceSceneIcons.js +++ b/js/common/view/WaveInterferenceSceneIcons.js @@ -1,5 +1,6 @@ // Copyright 2018, University of Colorado Boulder +//REVIEW very odd to have a class for this, especially since the icons are not at all related. Factory pattern would be better. /** * Creates a set of uniformly-sized icons for each of the scenes. * diff --git a/js/common/view/WaveInterferenceSlider.js b/js/common/view/WaveInterferenceSlider.js index 3dd88807..5e3c3d3e 100644 --- a/js/common/view/WaveInterferenceSlider.js +++ b/js/common/view/WaveInterferenceSlider.js @@ -33,7 +33,7 @@ define( require => { class WaveInterferenceSlider extends HSlider { /** - * @param {Property} property + * @param {Property} property //REVIEW {NumberProperty} ? * @param {number} min * @param {number} max */ diff --git a/js/common/view/WaveInterferenceText.js b/js/common/view/WaveInterferenceText.js index 264546ac..e345397e 100644 --- a/js/common/view/WaveInterferenceText.js +++ b/js/common/view/WaveInterferenceText.js @@ -22,6 +22,8 @@ define( require => { constructor( string, options ) { super( string, _.extend( { font: WaveInterferenceConstants.DEFAULT_FONT, + + //REVIEW This seems like a sketchy thing to set for all Text in the sim. Why this value? Why handle it this way? maxWidth: 120 }, options ) ); } diff --git a/js/common/view/WaveMeterProbeNode.js b/js/common/view/WaveMeterProbeNode.js index c967fc8f..0dddc606 100644 --- a/js/common/view/WaveMeterProbeNode.js +++ b/js/common/view/WaveMeterProbeNode.js @@ -27,8 +27,11 @@ define( require => { scale: 0.4, drag: () => {} }, options ); + super( options ); + visibleBoundsProperty.link( visibleBounds => this.setCenter( visibleBounds.closestPointTo( this.center ) ) ); + this.addInputListener( new DragListener( { translateNode: true, dragBoundsProperty: visibleBoundsProperty, diff --git a/js/interference/view/InterferenceScreenView.js b/js/interference/view/InterferenceScreenView.js index 84b6ad6d..e0781fc8 100644 --- a/js/interference/view/InterferenceScreenView.js +++ b/js/interference/view/InterferenceScreenView.js @@ -40,6 +40,7 @@ define( require => { const soundSeparationProperty = model.soundScene.sourceSeparationProperty; const lightSeparationProperty = model.lightScene.sourceSeparationProperty; + //REVIEW factor this ToggleNode out into its own class file? // Switch between controls for each scene. No advantage in using SceneToggleNode in this case // because the control constructor calls are substantially different. const toggleNode = new ToggleNode( model.sceneProperty, [ { @@ -68,6 +69,7 @@ define( require => { majorTicks: createTicks( lightSceneRange, [ waterSceneRange, soundSceneRange, lightSceneRange ] ) }, WaveInterferenceConstants.NUMBER_CONTROL_OPTIONS ) ) } ] ); + super( model, alignGroup, { // The pulse option does not appear in the Interference screen, because it is distracting and does not meet a @@ -85,6 +87,7 @@ define( require => { * they don't jitter when changing scenes, see https://github.com/phetsims/wave-interference/issues/214 * @param {string} string - the string to display * @param {string[]} allStrings - the strings for each scene, for layout + * @returns {Node} */ const createTickMarkLabel = ( string, allStrings ) => { const textNodes = allStrings.map( s => new WaveInterferenceText( s, { diff --git a/js/slits/view/BarriersNode.js b/js/slits/view/BarriersNode.js index 6e93b79a..95dc02fc 100644 --- a/js/slits/view/BarriersNode.js +++ b/js/slits/view/BarriersNode.js @@ -31,6 +31,7 @@ define( require => { assert && assert( model instanceof SlitsScreenModel ); + //REVIEW move duplicated cornerRadius constructor args to options.cornerRadius /** * Creates one of the 3 recycled rectangles used for rendering the barriers. */ @@ -77,6 +78,7 @@ define( require => { var modelBounds = scene.modelToLatticeTransform.viewToModelBounds( latticeBounds ); var tempViewBounds = this.modelViewTransform.modelToViewBounds( modelBounds ); + //REVIEW missing visibility annotation this.latticeToViewTransform = ModelViewTransform2.createRectangleMapping( latticeBounds, tempViewBounds ); this.addInputListener( new DragListener( { @@ -117,7 +119,8 @@ define( require => { } /** - * @private - update the shapes and text when the rotationAmount has changed + * Update the shapes and text when the rotationAmount has changed + * @private */ update() { const barrierType = this.scene.barrierTypeProperty.get(); diff --git a/js/slits/view/PlaneWaveGeneratorNode.js b/js/slits/view/PlaneWaveGeneratorNode.js index 552b2814..57727058 100644 --- a/js/slits/view/PlaneWaveGeneratorNode.js +++ b/js/slits/view/PlaneWaveGeneratorNode.js @@ -47,6 +47,7 @@ define( require => { center: verticalCylinderImageNode.center } ); + //REVIEW assert( !options.child ) or your children may be blown away by the constructor caller options = _.extend( { children: [ verticalCylinderImageNode, button ] }, options ); this.mutate( options ); diff --git a/js/slits/view/SlitsControlPanel.js b/js/slits/view/SlitsControlPanel.js index 95589902..e075e1f3 100644 --- a/js/slits/view/SlitsControlPanel.js +++ b/js/slits/view/SlitsControlPanel.js @@ -1,5 +1,6 @@ // Copyright 2018, University of Colorado Boulder +//REVIEW all of the ranges herein belong in the model, not in the view. They should be set on Scene.slitWidthProperty, where they will be needed for PhET-iO. /** * Controls for the barrier/slits. * @@ -44,6 +45,7 @@ define( require => { derive: 'barrierTypeProperty', bidirectional: true } ); + const comboBox = new ComboBox( [ ComboBox.createItem( new WaveInterferenceText( noBarrierString ), BarrierTypeEnum.NO_BARRIER ), ComboBox.createItem( new WaveInterferenceText( oneSlitString ), BarrierTypeEnum.ONE_SLIT ), diff --git a/js/slits/view/TheoryInterferenceOverlay.js b/js/slits/view/TheoryInterferenceOverlay.js index 5b320618..12d48831 100644 --- a/js/slits/view/TheoryInterferenceOverlay.js +++ b/js/slits/view/TheoryInterferenceOverlay.js @@ -19,7 +19,7 @@ define( require => { // constants const LENGTH = 500; const MAXIMUM_COLOR = 'yellow'; - const MINIMUM_COLOR = 'red'; + const MINIMUM_COLOR = 'red'; //REVIEW should this be PhetColorScheme.RED_COLOR_BLIND? const LINE_WIDTH = 1; class TheoryInterferenceOverlay extends Node { diff --git a/js/wave-interference-main.js b/js/wave-interference-main.js index c0bdeda5..791f4ffc 100644 --- a/js/wave-interference-main.js +++ b/js/wave-interference-main.js @@ -41,6 +41,7 @@ define( require => { // Elements should have the same widths but not constrained to have the same heights matchVertical: false } ); + const sim = new Sim( waveInterferenceTitleString, [ new WavesScreen( alignGroup ), new InterferenceScreen( alignGroup ), diff --git a/js/waves/view/WavesScreenView.js b/js/waves/view/WavesScreenView.js index 70ba4323..517d05ef 100644 --- a/js/waves/view/WavesScreenView.js +++ b/js/waves/view/WavesScreenView.js @@ -463,6 +463,7 @@ define( require => { } else { + //REVIEW #272 clarify emitter in this context // @protected - placeholder for alternative emitter nodes this.emitterLayer = new Node(); this.addChild( this.emitterLayer );