phetsims / normal-modes

"Normal Modes" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
0 stars 2 forks source link

factor out duplicated RadioButtonGroup #48

Closed pixelzoom closed 4 years ago

pixelzoom commented 4 years ago

Related to #2 (code review).

This RadioButtonGroup appears in both screens:

screenshot_112

The code to create it is duplicated in AmpPhaseAccordionBox and AmpSelectorAccordionBox:

      const horizontalButton = new ArrowNode( 0, 0, iconSize, 0, AXES_ARROW_OPTIONS );
      const verticalButton = new ArrowNode( 0, 0, 0, iconSize, AXES_ARROW_OPTIONS );
      const directionOfMotionRadioButtonGroup = new RadioButtonGroup( model.directionOfMotionProperty, [ {
        value: model.directionOfMotion.HORIZONTAL,
        node: horizontalButton
      }, {
        value: model.directionOfMotion.VERTICAL,
        node: verticalButton
      } ], {
        deselectedLineWidth: 1,
        selectedLineWidth: 1.5,
        cornerRadius: 8,
        deselectedButtonOpacity: 0.35,
        buttonContentXMargin: 8,
        buttonContentYMargin: 8,
        orientation: 'vertical'
      } );

Factor this out and encapsulate in a class that extends RadioButtonGroup.

Hyodar commented 4 years ago

Encapsulated the RadioButtonGroup in a DirectionOfMotionRadioButtonGroup class in the latest commit. How do you feel about the DirectionOfMotion name? I think it only makes sense on the 1D mode, as the masses do move in both directions on 2D mode. Maybe changing DirectionOfMotion to AmplitudeDirection (and all associated names in the same way, 1D and 2D) would be better?

pixelzoom commented 4 years ago

@Hyodar renaming DirectionOfMotion to AmplitudeDirection sounds like a good change. And the radio button group would then be AmplitudeDirectionRadioButtonGroup.

The current implementation of DirectionOfMotionRadioButtonGroup looks very nice. You might consider renaming horizontalButton and verticalButton to something like horizontalButtonDescription and verticalButtonDescription. They are not buttons, they are descriptions that RadioButtonGroup will use to instantiate buttons.

Hyodar commented 4 years ago

Makes sense. Renamed both DirectionOfMotion to AmplitudeDirection and the buttons to button descriptors.

pixelzoom commented 4 years ago

@Hyodar are you testing your changes with assertions enabled? There's a bug in AmplitudeDirectionRadioButtonGroup.js (line 45):

45        }, options
46      } );

The options argument needs to be moved down 1 line:

45        }
46      }, options );

This is causing merge to fail with this error:

assert.js?bust=1581460495408:22 Uncaught Error: Assertion failed: at least one source expected
    at window.assertions.assertFunction (assert.js?bust=1581460495408:22)
    at merge (merge.js?bust=1581460495492:30)
    at new AmplitudeDirectionRadioButtonGroup (AmplitudeDirectionRadioButtonGroup.js?bust=1581460495492:28)
    at new NormalModeSpectrumAccordionBox (NormalModeSpectrumAccordionBox.js?bust=1581460495492:214)
    at new OneDimensionScreenView (OneDimensionScreenView.js?bust=1581460495492:87)
    at OneDimensionScreen.createView (OneDimensionScreen.js?bust=1581460495492:36)
    at OneDimensionScreen.initializeView (Screen.js?bust=1581460495492:266)
    at Array.<anonymous> (Sim.js?bust=1581460495492:866)

It's best to always test with query parameter ea to enable assertions. And run grunt lint to check with ESlint before pushing changes.

Hyodar commented 4 years ago

Oh, sorry. My bad. Fixing right away.

pixelzoom commented 4 years ago

No problem! We understand that it takes a little while to get used to PhET processes. And we appreciate your work.

Hyodar commented 4 years ago

Probably everything fixed by now.

pixelzoom commented 4 years ago

Everything looks great, so I'll close this.