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 support for dynamic locale #319

Closed
3 tasks
pixelzoom opened this issue Aug 22, 2022 · 8 comments
Closed
3 tasks

Add support for dynamic locale #319

pixelzoom opened this issue Aug 22, 2022 · 8 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Aug 22, 2022

@jonathanolson @samreid and I had a discussion about dynamic locale and dynamic strings, and what needs to be done to support them. They suggested that I start with one sim, and Natural Selection seems to make the most sense, since it's next on the list in https://github.com/orgs/phetsims/projects/44.

To add support:

  • Convert from string to TReadonlyProperty<string> instances from strings file. Do this everywhere that a string is used. Search for "naturalSelectionStrings." There are 93 occurrences.

  • Use DerivedProperty for strings that involve a pattern. Search for "{{" in natural-selection-strings_en.json. There are 5 occurrences.

  • Run with ?locales=* to enable the locale-testing button (globe) in the navigation bar. Open the locale dialog by pressing the globe button in the navigation bar. Press and hold while moving the pointer over locale names. This changes the locale. Watch for layout problems. Fix any layout problems that are identified. For example, if a Text node needs to remain centered on something, then a boundsProperty listener may be needed.

@amanda-phet @kathy-phet FYI.

@pixelzoom pixelzoom self-assigned this Aug 22, 2022
@pixelzoom pixelzoom changed the title Convert to dynamic strings Add support for dynamic locale Aug 22, 2022
@pixelzoom
Copy link
Contributor Author

I made an initial pass at this. What I discovered so far:

  • The globe button (?locale=*) has been removed from the navbar. The recommend way to test is to change localeProperty in Studio using arrow keys to quicky switch between radio buttons.
  • overrides.js file will frequently need to be changed manually.
  • In addition to changing from string to Property values, extensive changes to sim-specific APIs are required to support that type change.
  • All buttons that involve translated strings need to be instantiated with widthSizable: true in order to resize correctly. (We should discuss whether resize should be opt-in or opt-out for UI components in genera.)
  • Anything that has alignment requirements (e.g. "center this button in the play area") needs to have a boundProperty listener or a layout manager that handles realignment when the text changes.
  • Adding support for dynamic layout involves changing from string types to Property types, so type-checking would be very helpful. Converting to TypeScript before adding dynamic-locale support is recommended (and approved by @kathy-phet).

pixelzoom added a commit that referenced this issue Aug 27, 2022
pixelzoom added a commit that referenced this issue Aug 27, 2022
pixelzoom added a commit that referenced this issue Aug 27, 2022
pixelzoom added a commit to phetsims/scenery-phet that referenced this issue Aug 27, 2022
pixelzoom added a commit that referenced this issue Aug 27, 2022
pixelzoom added a commit that referenced this issue Aug 27, 2022
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Aug 27, 2022

I punted on TypeScript conversion (#320) due to the effort required, and proceeded with adding support for dynamic locale.

I made good progress, and I'd estimate that this is ~90% done.

I had to undo a significant amount of work because of phetsims/chipper#1314, and I've added TODO comments in the code to revisit.

I still need to figure out how to make GenePair.getGenotypeAbbreviation dynamic -- that's going to be complicated. See TODO in the code.

One thing I learned during this conversion is that creating a DerivedProperty every Text/RichText is not alway the best solution. When there are multiple Text/RichText instances that have the same dependencies, and need to all be updated at the same time, it seems better to use a Multilink. See for example ProportionsBarNode.js

There are also cases where dependencies are not created until after the Text/RichText is created, and Multilink is useful there too.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Aug 30, 2022

Adding links from all Text nodes to their associated string Properties was a huge task. And I'm wondering about the value of doing this - it feels very over-engineered to me. In any case, it's done.

I also replaced visiblePropertyOptions: { phetioReadOnly: true } with the newer phetioVisibilePropertyInstrumented: false for cases where the visibileProperty does not need to be instrumented, because it can't be changed through by the sim or API, so inspecting it has no value.

There are 2 TODOs in the code that still need to be addressed, and they require common code work in phetsims/chipper#1314 and phetsims/sun#746.

So... I think this is at a place where it's ready for review by @amanda-phet and @kathy-phet.

@pixelzoom
Copy link
Contributor Author

Notes from 9/1/2022 design meeting:

@arouinfar
Copy link

@pixelzoom I reviewed on master with stringTest=dynamic. I also used Studio to hide controls to exercise dynamic layout (since it normally goes hand-in-hand with dynamic locale). Things are looking pretty good, with just one exception.

The "No Data" title in the Proportions graph becomes uncentered when the string expands in size.
Screen Shot 2023-07-21 at 12 03 48 PM

@pixelzoom
Copy link
Contributor Author

"No Data" layout was fixed in the above commit. @arouinfar please review, close if OK.

@pixelzoom pixelzoom assigned arouinfar and unassigned pixelzoom Jul 21, 2023
@arouinfar
Copy link

Looks good on master, thanks @pixelzoom. 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

4 participants