Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

duplication in options to HSlider #45

Closed
pixelzoom opened this issue Mar 5, 2016 · 1 comment
Closed

duplication in options to HSlider #45

pixelzoom opened this issue Mar 5, 2016 · 1 comment
Assignees

Comments

@pixelzoom
Copy link
Contributor

Related to #30 (code review).

In AtomPropertiesPanel, there's a lot of duplication in the options argument passed to the 2 HSliders.

Recommend to factor out the duplication, like this:

var sliderOptions = {
      trackFill: RSConstants.PANEL_SLIDER_FILL_COLOR,
      trackStroke: RSConstants.PANEL_SLIDER_FILL_COLOR,
      majorTickStroke: RSConstants.PANEL_SLIDER_FILL_COLOR,
      majorTickLength: 15,
      tickLabelSpacing: 2,
      trackSize: new Dimension2( sliderWidth, 1 ),
      thumbSize: RSConstants.PANEL_SLIDER_THUMB_DIMENSION,
      thumbCenterLineStroke: 'white',
      startDrag: function() { // called when the pointer is pressed
        model.userInteraction = true;
      },
      endDrag: function() { // called when the pointer is released
        model.userInteraction = false;
      }
    }

Then fill in the things that are specific to each slider, like this:

 var protonCountSlider = new HSlider( model.protonCountProperty, {
      min: RSConstants.MIN_PROTON_COUNT,
      max: RSConstants.MAX_PROTON_COUNT
    }, _.extend( {}, sliderOptions, {
      thumbFillEnabled: 'rgb(220, 58, 10)',
      thumbFillHighlighted: 'rgb(270, 108, 60)'
    } );

Similarly for neutronCountSlider.

Take special note of how _.extend is being used here (3 args), so that sliderOptions is not modified.

Note also that some of this duplication also occurs in AlphaParticlePropertiesPanel, but it's less of a concern. Up to you whether you want to address it there, and factor out a common set of options for all HSliders that you put in RSConstants.

schmitzware pushed a commit that referenced this issue Mar 7, 2016
And a bunch of auto IntelliJ style changes - sorry.
@schmitzware schmitzware assigned pixelzoom and unassigned schmitzware Mar 7, 2016
@pixelzoom
Copy link
Contributor Author

Looks good, closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants