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

Instrument or Remove Dynamic Locale #372

Closed
Nancy-Salpepi opened this issue May 4, 2023 · 11 comments
Closed

Instrument or Remove Dynamic Locale #372

Nancy-Salpepi opened this issue May 4, 2023 · 11 comments

Comments

@Nancy-Salpepi
Copy link

Test device
MacBook Air M1 chip

Operating System
13.3.1

Browser
Safari

Problem description
For phetsims/qa#936, dynamic locale hasn't been instrumented. Should it be part of this current dev version or should the preferences dialog be removed?

Currently, when I switch locales from the Preferences menu, longer translations cause the Reset All button to move partially/completely offscreen. Also, with stringTest=dynamic, none of the words in the panels change.

@KatieWoe
Copy link
Contributor

KatieWoe commented May 5, 2023

Images as example. Interestingly, changing the window size seems to fix some of these issues, but it is definitely still a bug.
Masses and Springs screenshot
Pružinový oscilátor screenshot

@KatieWoe
Copy link
Contributor

KatieWoe commented May 5, 2023

In addition, most of the strings in the sim are not translated when switching locale dynamically.

@arouinfar
Copy link

Dynamic strings/locale is not within the scope of this release, so I think we should hide the Preferences dialog.

@jbphet
Copy link
Contributor

jbphet commented May 9, 2023

I looked into whether it is possible to just turn off the preferences dialog for the "all" version of the sim, which is what we publish on the main site nowadays. It doesn't seem like it is. I was looking at this commit, and from a cursory analysis, it appears to set allowLocaleSwitching to true for builds of the "all" version of the file. I asked the author of the commit about this over slack, and here's a section of the dialog:

@jbphet: If I'm reading this right, it means that allowLocaleSwitching is always enabled for the "all" version of a sim. I need to republish Masses and Springs from master, and this change seems to imply that I (or someone) has to implement string Properties and dynamic layout in the sim before publishing, which feels like a big ask. Is this intentional? In other words, is our policy now that anything published off of master has to be upgraded to support dynamic locale switching?
@jonathanolson: That's a good policy question. I don't think that we'd need a HARD constraint of "you have to make something dynamic if publishing from master", but it sounds nice to do if possible. I'm sure we could come up with a technical way to default it to false for certain simulations.

Based on all of this, I don't think there is currently an easy way to turn off locale switching. I will discuss with the rest of the dev group whether we want to add such an option or require support for dynamic strings in all publications off of master.

@jessegreenberg
Copy link
Contributor

Hey there, I just noticed this thread go by. PreferencesModel has this option to disable dynamic locales for the sim, see https://github.com/phetsims/energy-skate-park/blob/044dc7a98ff33aef0270f91f7dafed092dc0c43d/js/energy-skate-park-main.js#L44-L47

@jbphet
Copy link
Contributor

jbphet commented May 9, 2023

Thank you @jessegreenberg! I just tried adding the following to the sim options and indeed, the preferences menu option disappears from the nav bar:

  preferencesModel: new PreferencesModel( {
    localizationOptions: {
      supportsDynamicLocales: false
    }
  } )

I'll also add an agenda item to the next dev meeting to make sure other developers are aware of this and on board with using it.

@samreid
Copy link
Member

samreid commented May 11, 2023

We reviewed this in today's dev meeting. We agree we can't always take the time to make all strings dynamic. So using this option to shut off the localization part of the preferences seems good.

MK: During RAP, I felt like we had to add the dynamic locales--did not really know there was an opt-out option. So we just went for it and it turned out ok. Opt-out is great though and we can determine that by priority on a case by case basis.

UPDATE: We saw that package.json has "supportsDynamicLocale": true and would like to see if that is the appropriate valve. It seems like the idea in the preceding comment is not connected to this package.json value. So you should be able to shut this off from package.json itself. @jbphet will take the lead. Thanks!

@zepumph would like to take a first look though. @zepumph will invite @jessegreenberg as appropriate. Will need defaults, and support for opt-in opt-out and query parameters, etc.

@zepumph
Copy link
Member

zepumph commented May 31, 2023

And let's also note the built side of things: window.phet.chipper.allowLocaleSwitching

zepumph added a commit to phetsims/masses-and-springs-basics that referenced this issue Jun 21, 2023
…ms/masses-and-springs#372

- rename from plural to singular for consistency,
- update documentation
- opt out via package.json instead of PreferencesModel
zepumph added a commit to phetsims/phet-info that referenced this issue Jun 21, 2023
zepumph added a commit to phetsims/phet-info that referenced this issue Jun 21, 2023
…ms/masses-and-springs#372

- rename from plural to singular for consistency,
- update documentation
- opt out via package.json instead of PreferencesModel
zepumph added a commit to phetsims/energy-skate-park that referenced this issue Jun 21, 2023
…ms/masses-and-springs#372

- rename from plural to singular for consistency,
- update documentation
- opt out via package.json instead of PreferencesModel
zepumph added a commit to phetsims/energy-skate-park-basics that referenced this issue Jun 21, 2023
…ms/masses-and-springs#372

- rename from plural to singular for consistency,
- update documentation
- opt out via package.json instead of PreferencesModel
zepumph added a commit that referenced this issue Jun 21, 2023
- rename from plural to singular for consistency,
- update documentation
- opt out via package.json instead of PreferencesModel
@zepumph
Copy link
Member

zepumph commented Jun 21, 2023

I worked with @jessegreenberg to make sure that this is still an 'opt out' default, but that the package string still supports strict mode.

I am really happy with this behavior. We were able to remove the redundant usages of specific calls to opt out of the preferences locale switcher. That is now controlled by the package.json flag.

In built versions, I was also glad to see that phet.chipper.allowLocaleSwitching was properly combining into the right cases.

In http://localhost:8080/masses-and-springs-basics/build/phet/masses-and-springs-basics_en_phet.html?debugger&locales=*&supportsDynamicLocale=true it still won't show the locale switcher because we only support english (and the build process passed that flag through.

In http://localhost:8080/energy-skate-park/build/phet/energy-skate-park_all_phet_debug.html we by default don't show the switcher, because we opted out in the package.json flag, but if you add ?supportsDynamicLocale=true, then we will still show it because we have other locales available.

@jessegreenberg can you please spot check and let me know if you have any other steps here.

zepumph added a commit to phetsims/joist that referenced this issue Jun 21, 2023
…ms/masses-and-springs#372

- rename from plural to singular for consistency,
- update documentation
- opt out via package.json instead of PreferencesModel
zepumph added a commit to phetsims/chipper that referenced this issue Jun 21, 2023
…ms/masses-and-springs#372

- rename from plural to singular for consistency,
- update documentation
- opt out via package.json instead of PreferencesModel
@zepumph zepumph assigned jessegreenberg and unassigned zepumph Jun 21, 2023
zepumph added a commit to phetsims/chipper that referenced this issue Jun 22, 2023
@zepumph
Copy link
Member

zepumph commented Jun 22, 2023

CT found one issue in scenery sandbox pages, phetsims/chipper@8232add

@jessegreenberg
Copy link
Contributor

Looks good @zepumph, thanks for taking this on. I appreciate the rename (dynamicLocales -> dynamicLocale) for consistency. I did a bit of testing for the logic in a built sim, combinations behaved as I expected. Thank you!

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