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

NavigationBarScreenButton doesn't extend JoistButton #273

Open
pixelzoom opened this issue Jul 8, 2015 · 8 comments
Open

NavigationBarScreenButton doesn't extend JoistButton #273

pixelzoom opened this issue Jul 8, 2015 · 8 comments

Comments

@pixelzoom
Copy link
Contributor

Other similar buttons (PhetButton, HomeButton) extend JoistButton. NavigationBarScreenButton extends Node, and adds its own highlights and associated logic, duplicating code that's in JoistButton.

@samreid Is there any reason why this is the case? Something I'm missing?

@samreid
Copy link
Member

samreid commented Jul 8, 2015

It may be possible to write NavigationBarScreenButton to extend JoistButton. It may be a good way to factor out some code at the moment, but my hesitation is that NavitaionBarScreenButton is more of a radio button and hence has a different feature set than the other JoistButtons.

@samreid samreid assigned pixelzoom and unassigned samreid Jul 8, 2015
@pixelzoom
Copy link
Contributor Author

So JoistButton is really JoistPushButton?

@pixelzoom
Copy link
Contributor Author

So JoistButton is really JoistPushButton?

Indeed it is. JoistButton.js line 43:
this.buttonModel = new PushButtonModel( options ); // @private

@pixelzoom
Copy link
Contributor Author

Recommended type hierarchy:

JoistButton
  JoistPushButton
    HomeButton
    PhetButton
  JoistRadioButton
    NavigationBarScreenButton

@samreid
Copy link
Member

samreid commented Jul 8, 2015

That seems like a nice intermediate solution, though perhaps a better long term solution would involve moving some of this to sun and/or integrating it with the sun button/radiobutton implementations.

@pixelzoom
Copy link
Contributor Author

Still seems like a lot of work. I'm going to defer this unless there's no other way that I can finish #241.

@pixelzoom
Copy link
Contributor Author

I was able to complete #241 without addressing this. So unassigning because this is unlikely to be addressed in the near future, and it's not clear that that I should be the one addressing it.

@pixelzoom pixelzoom removed their assignment Jul 13, 2015
@pixelzoom
Copy link
Contributor Author

Still a problem.... In general, joist really needs a full overhaul of its buttons (home screen, nav bar, etc.)

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

3 participants