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

Should a 'ControlPanel.js' Type be factored out? #60

Closed
zepumph opened this issue Jul 18, 2017 · 5 comments
Closed

Should a 'ControlPanel.js' Type be factored out? #60

zepumph opened this issue Jul 18, 2017 · 5 comments

Comments

@zepumph
Copy link
Member

zepumph commented Jul 18, 2017

from code review #30
I see a lot of duplicated code in all three of the control panels. Perhaps there is a way to save code by factoring that out into its own file. jsinspect noticed multiple sections in all three control panels that were basically the exact same.

@pixelzoom
Copy link
Contributor

Factoring out ControlPanel.js feels a bit "forced" to me. There are significant differences between all 3 control panels, especially the set of "View" check boxes. What they share is parts of the control panel, specifically the "Surface" and "Electric Field" controls. I'll look at factoring out SurfaceControl.js and ElectricFieldControl.js, then use those in the existing control panels.

@zepumph
Copy link
Member Author

zepumph commented Jul 18, 2017

I definitely understand about the "forced" feeling. Your plan sounds like a good one. If we find that it isn't helpful than I think it is fine how it is.

@pixelzoom
Copy link
Contributor

pixelzoom commented Jul 19, 2017

Major changes to control panels, see above commits.

Summary of new types:

MPControlPanel - used throughout, responsible for inserting horizontal separators
SurfaceTypeControl - title and radio buttons for selecting the surface type
EFieldControl - title and on/off button for e-field
TwoAtomsViewControls - 'View' title and check boxes for Two Atoms
ThreeAtomsViewControls - 'View' title and check boxes for Three Atoms
RealMoleculesViewControls - 'View' title and check boxes for Real Molecules

Example usage in TwoAtomsScreenView:

    var controlPanel = new MPControlPanel( [
      new TwoAtomsViewControls( viewProperties ),
      new SurfaceTypeControl( viewProperties.surfaceTypeProperty ),
      new EFieldControl( model.eField.enabledProperty )
    ] );

@zepumph please review.

@pixelzoom
Copy link
Contributor

pixelzoom commented Jul 19, 2017

Btw... I compared to 1.0.0-dev.24 (looks identical) and tested with
?ea&realMolecules&stringTest=long

pixelzoom added a commit that referenced this issue Jul 19, 2017
pixelzoom added a commit that referenced this issue Jul 19, 2017
pixelzoom added a commit that referenced this issue Jul 19, 2017
pixelzoom added a commit that referenced this issue Jul 19, 2017
pixelzoom added a commit that referenced this issue Jul 19, 2017
@zepumph
Copy link
Member Author

zepumph commented Jul 19, 2017

This looks really nice. Thanks, closing

@zepumph zepumph closed this as completed Jul 19, 2017
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