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

what to do about old-style format patterns? #266

Closed
pixelzoom opened this issue Dec 18, 2018 · 7 comments
Closed

what to do about old-style format patterns? #266

pixelzoom opened this issue Dec 18, 2018 · 7 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Dec 18, 2018

Noted in code review #259, and related to general issue phetsims/scenery-phet#446.

Wave Interference is a new sim, but is using old-style format patterns (numbered placeholders), i.e.:

  "cmValue": {
    "value": "{0} cm"
  },
  "nmValue": {
    "value": "{0} nm"
  },

Creating more strings that use old-style seems like something that we want to avoid. Whether we do anything for Wave Interference depends on decisions for phetsims/scenery-phet#446.

@samreid
Copy link
Member

samreid commented Dec 18, 2018

Should we be using this pattern for now? phetsims/scenery-phet#317 (comment)

@pixelzoom
Copy link
Contributor Author

If you need/want a quick fix, yes, handle the conversion from named to numbered placeholders in your code. And/or wait to see what is decided in phetsims/scenery-phet#446.

@samreid
Copy link
Member

samreid commented Dec 20, 2018

On hold until phetsims/scenery-phet#446 is decided.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Dec 22, 2018

12/20/18 dev meeting consensus:

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Dec 28, 2018

12/28/18 update: NumberDisplay (and therefore NumberControl) now supports both named and numbered placeholders, see phetsims/scenery-phet#446 (comment). So there is no need to apply the workaround in this sim. You can simply use {{value}} in your English translated strings, and pass them to NumberControl.

samreid added a commit that referenced this issue Jan 2, 2019
@samreid
Copy link
Member

samreid commented Jan 2, 2019

Proposed fix is pushed, @pixelzoom can you please review? By the way, thanks for fixing that!

@pixelzoom
Copy link
Contributor Author

👍 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