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

Add screen name to title when multiscreen sim is run with one screen via ?screens=3 #449

Closed
zepumph opened this issue Oct 27, 2017 · 34 comments

Comments

@zepumph
Copy link
Member

zepumph commented Oct 27, 2017

From #445:

when multiscreen sim and you specify 1 screen with ?screens=2, then update the title in the nav bar to include the screen name: "Forces and Motion: Basics — Motion"

This seems like a nice improvement across the board, and will be especially nice is PhET-iO. I will take a stab at this and then ask for review since it touches every multiscreen sim.

@zepumph
Copy link
Member Author

zepumph commented Oct 27, 2017

Commit is above. @samreid will you please review.

@samreid
Copy link
Member

samreid commented Oct 27, 2017

I suspect we will need to internationalize this using a pattern so it will work well for RTL languages as well.

@zepumph
Copy link
Member Author

zepumph commented Oct 27, 2017

How about that implementation? Especially the name of the string key, I feel like I'm bad at naming those things.

@samreid
Copy link
Member

samreid commented Oct 27, 2017

There is a lint error:

/Users/samreid/github/joist/js/NavigationBar.js
45:3 error Mismatched var in require(string!), key=simTitleWithScreenNamePattern, var=simTitleWithScreenNamePattern, desiredVar=simTitleWithScreenNamePatternString string-require-statement-match

@samreid
Copy link
Member

samreid commented Oct 27, 2017

Everything else looks good to me.

@samreid samreid removed their assignment Oct 27, 2017
@samreid
Copy link
Member

samreid commented Oct 27, 2017

Do @kathy-phet and @ariel-phet want maintenance releases of this feature for our LOL sims?

@kathy-phet
Copy link

It's not a high priority, but if it can be done without much difficulty, it seems worthwhile. Not time sensitive either.

@zepumph
Copy link
Member Author

zepumph commented Oct 27, 2017

@samreid I'm very un-involved in the LOL process, what sims would need this if we decide it is worth the cost?

zepumph added a commit that referenced this issue Oct 27, 2017
@zepumph
Copy link
Member Author

zepumph commented Oct 27, 2017

@samreid I see #450, want to just add these commits into that?

a3e46b8
d418f9e
28a9c13

@samreid
Copy link
Member

samreid commented Nov 2, 2017

@zepumph will prevent cases like "Concentration -- Concentration"

@samreid
Copy link
Member

samreid commented Nov 2, 2017

Or it should error out if you put ?screens=... for a one screen sim?

UPDATE: we decided it should be an error if someone specifies ?screens for a single screen sim.

@pixelzoom
Copy link
Contributor

11/2/17 dev meeting:

Do this only in the navbar, not in the About dialog or Report A Problem. Also not in the browser title because that may mess up statistics.

@kathy-phet
Copy link

Sounds decided - only in the nav bar.

@samreid
Copy link
Member

samreid commented Nov 3, 2017

@zepumph anything else to do for this issue other than #449 (comment)
?

Also, please unhold #450 when complete.

@jonathanolson
Copy link
Contributor

phetsims/pendulum-lab#197 also waiting for this (it looks like).

@zepumph
Copy link
Member Author

zepumph commented Nov 3, 2017

Looks good, closing!

@zepumph zepumph closed this as completed Nov 3, 2017
@jonathanolson
Copy link
Contributor

Is there a list of commits to cherry-pick to apply this to a maintenance release branch?

@jonathanolson jonathanolson reopened this Nov 3, 2017
@zepumph
Copy link
Member Author

zepumph commented Nov 3, 2017

#449 (comment)

@jonathanolson
Copy link
Contributor

Thanks!

@zepumph
Copy link
Member Author

zepumph commented Nov 3, 2017

I'm pretty sure that we decided that we decided that we weren't going to update the title anywhere else but the nav bar title. This decision included the 'report a problem' report, but I wanted to make sure that this was alright with @phet-steele. @phet-steele please close if this sounds good to you.

@phet-steele
Copy link

@phet-steele please close if this sounds good to you.

Yup, just making sure we covered all of our bases.

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

8 participants