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

navbar and home screen layout are messed up due to dependencies on magic numbers #185

Closed
pixelzoom opened this issue Dec 23, 2014 · 7 comments

Comments

@pixelzoom
Copy link
Contributor

In #126, ScreenView.DEFAULT_LAYOUT_BOUNDS was changed from (0,0,768,504) to (0,0,1024,618). This exposed dependencies and assumptions elsewhere in joist about the layoutBounds. Many aspects of the home screen and navigation bar are affected, including:

• size of the navbar
• size of icons and labels
• spacing of icons and labels
• positioning of the PhET menu on the home screen

768, 504 and 40 (navbar height) appear in multiple places as magic numbers, with no association to ScreenView.DEFAULT_LAYOUT_BOUNDS. Font sizes also appear to have been selected based on these magic numbers, and will likely look smaller at the new DEFAULT_LAYOUT_BOUNDS. Here are some examples:

Sim:

518 //Use Mobile Safari layout bounds to size the home screen and navigation bar
519 var scale = Math.min( width / 768, height / 504 );
524 //40 px high on Mobile Safari
535 var navBarHeight = scale * 40;

NavigationBar:

35 this.navBarScale = 1;
36 this.navBarWidth = 768;
37 this.navBarHeight = 40;
50 this.titleLabel = new Text( sim.name, { font: new PhetFont( 18 ), pickable: false } );

@pixelzoom
Copy link
Contributor Author

I took a couple of stabs at fixing this (see commits/reverts towards the end of #126), but fixing one thing (e.g. location of the PhET menu on the home screen) would break something else (e.g., size of navbar). There should clearly be a dependency on ScreenView.DEFAULT_LAYOUT_BOUNDS. And there are clearly some dependencies and magic numbers that should not be in this implementation.

Since @samreid was the original author, assigning to him.

pixelzoom added a commit to phetsims/least-squares-regression that referenced this issue Dec 23, 2014
…ounds, set to those bounds explicitly instead of using DEFAULT_LAYOUT_BOUNDS, until phetsims/joist#185 is fixed
pixelzoom added a commit to phetsims/sugar-and-salt-solutions that referenced this issue Dec 23, 2014
…ounds, set to those bounds explicitly instead of using DEFAULT_LAYOUT_BOUNDS, until phetsims/joist#185 is fixed
pixelzoom added a commit that referenced this issue Dec 23, 2014
…those bounds explicitly instead of using DEFAULT_LAYOUT_BOUNDS, until #185 is fixed
@pixelzoom
Copy link
Contributor Author

I'm in the late stages of RPAL development, and I need to keep moving. So until this is resolved, I've set ScreenView.DEFAULT_LAYOUT_BOUNDS back to (0,0,768,504) in 06949b1. You can reproduce the problems described here by setting it to (0,0,1024,618). See ScreenView lines 24 & 25.

@samreid
Copy link
Member

samreid commented Dec 26, 2014

How about this solution for now?

1. Change back to the new value in ScreenView:

var DEFAULT_LAYOUT_BOUNDS = new Bounds2( 0, 0, 1024, 618 );

2. Supply the original bounds in HomeScreen.js

ScreenView.call( this, {renderer: 'svg', layoutBounds: new Bounds2( 0, 0, 768, 504 )} );

Please review and let me know your thoughts.

@samreid
Copy link
Member

samreid commented Dec 26, 2014

Whatever change we apply to solve this problem (whether the one proposed or another one), we should apply the solution to joist/master and joist/ohtwo.

@samreid samreid assigned pixelzoom and unassigned samreid Dec 26, 2014
@pixelzoom
Copy link
Contributor Author

@samreid's solution (above) sounds good. I'll handle this, and try to clean up the magic numbers too.

pixelzoom added a commit that referenced this issue Dec 31, 2014
…ies on magic numbers throughout HomeScreen and NavigationBar
@pixelzoom
Copy link
Contributor Author

I set HomeScreen's layoutBounds to (0,0,768,504), then replaced magic numbers in NavigationBar with references to the dimensions of sim.homeScreen.layoutBounds. Tested with a few different sims and everything looked OK. Assigning to @samreid for review.

@pixelzoom pixelzoom assigned samreid and unassigned pixelzoom Dec 31, 2014
pixelzoom added a commit that referenced this issue Dec 31, 2014
…d fix navbar scaling when resizing browser window
pixelzoom added a commit that referenced this issue Dec 31, 2014
@samreid
Copy link
Member

samreid commented Jan 5, 2015

I skimmed @pixelzoom change sets and tested a single-screen sim and a multi-screen sim and all seems 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