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

Avoid using ToggleNode in the NavigationBarScreenButton #161

Closed
samreid opened this issue Sep 15, 2014 · 5 comments
Closed

Avoid using ToggleNode in the NavigationBarScreenButton #161

samreid opened this issue Sep 15, 2014 · 5 comments

Comments

@samreid
Copy link
Member

samreid commented Sep 15, 2014

In phetsims/energy-skate-park-basics#208, I said:

I tried getting @jonathanolson's changes to NavigationBarScreenButton from joist/ohtwo and this reduced the number of nodes from 509 to 344. In my opinion, that is very significant. I'll look into merging that commit into master.

@samreid
Copy link
Member Author

samreid commented Sep 15, 2014

Note, the work was done by @jonathanolson in f31cde3, and just needs to be merged in from ohtwo to master. I'll try git cherry-pick, but I haven't used cherry-pick before and not sure if it is the perfect thing.

git cherry-pick f31cde3

@samreid
Copy link
Member Author

samreid commented Sep 15, 2014

After running the command above, the system reported:

[master e09cf43] NavigationBarScreenButton rewrite so we don't have 8 copies of our icon per screen.  Significantly smaller scene graph for some sims. Possibly
relates to #137, #108, #94, #80.
 Author: Jonathan Olson <[email protected]>
 1 file changed, 80 insertions(+), 64 deletions(-)
 rewrite js/NavigationBarScreenButton.js (62%)

Tests seemed good, so I'll push shortly.

@samreid
Copy link
Member Author

samreid commented Sep 15, 2014

I pushed the cherry-pick to master above, which was committed as e09cf43

@samreid
Copy link
Member Author

samreid commented Sep 15, 2014

Everything above seems promising, though there is the possibility we could run into issues when running a more full merge to ohtwo in the future.

@samreid
Copy link
Member Author

samreid commented Oct 3, 2014

I'll close this issue now since the rewritten buttons are in master.

@samreid samreid closed this as completed Oct 3, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant