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

incorrect keys for screen titles #249

Closed
pixelzoom opened this issue Jul 31, 2019 · 4 comments
Closed

incorrect keys for screen titles #249

pixelzoom opened this issue Jul 31, 2019 · 4 comments

Comments

@pixelzoom
Copy link
Contributor

Related to code review #247.

From code-review checklist:

(4) String keys for screen names should have the general form "screen.{{screenName}}". E.g.:

  "screen.explore": {
    "value": "Explore"
  },

This sim does not follow that convention:

  "intro": {
    "value": "Intro"
  },
  "systems": {
    "value": "Systems"
  },
@chrisklus
Copy link
Contributor

@pixelzoom do you know how important this convention is? For example, would the lack of screen. be causing some metadata service we have to not find EFAC screen names?

This sim has a lot of translations, so I think it would be a difficult change to make.

@chrisklus chrisklus assigned pixelzoom and unassigned chrisklus Aug 1, 2019
@pixelzoom
Copy link
Contributor Author

I don't know if there is anything else that relies on this convention. Probably a good question for @ariel-phet and someone familiar with website, etc.

@pixelzoom pixelzoom assigned ariel-phet and unassigned pixelzoom Aug 2, 2019
@ariel-phet
Copy link

@jbphet in a slack conversation said:

I know that the sim title string is used outside of the sim, but I'm not sure that anything else is. I think this convention was missed because the screen titles were one of the first things done, probably way back when Martin started the very first initial port, before the convention existed. The sim seems to have been published without it, so it's probably fine to leave it as is.

Given how many translations we have, it seems best not to mess with this right now.

@chrisklus I believe you cannot comment in the strings file, but if there is another way to note why it is being left this way somewhere in the code, that would be best.

@chrisklus
Copy link
Contributor

I added a note in EFACConstants.js, 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

3 participants