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

Support dynamic locales #399

Closed
samreid opened this issue Mar 2, 2023 · 5 comments
Closed

Support dynamic locales #399

samreid opened this issue Mar 2, 2023 · 5 comments

Comments

@samreid
Copy link
Member

samreid commented Mar 2, 2023

During code review #398, @matthew-blackman @jessegreenberg and I discovered that the sim strings do not dynamically adjust when the locale is changed at runtime. We tested this via ?stringTest=dynamic then using the arrow keys. The reviewers recommend that we should take the time now to support dynamic locale. It does not look like a very large number of strings on the screen.

@jessegreenberg indicated a previous decision that we did not have time to support dynamic strings for this sim.

@jessegreenberg indicated that this is also moving toward a production publication, not a prototype publication. For me, this is even more reason to support dynamic strings.

@jessegreenberg wanted to run this past @kathy-phet before spending time on it.

@kathy-phet
Copy link

Yes, please support dynamic locale. @jessegreenberg - You have my support/time to spend time on this. Thanks!

@jessegreenberg
Copy link
Contributor

OK, thanks @kathy-phet! I have added this to the dim sum items for this iteration. This should be a quick task but we will report back if it gets very complicated due to the large number of Voicing strings.

@samreid samreid removed their assignment Mar 7, 2023
@jessegreenberg
Copy link
Contributor

string -> StringProperty done in the above commit, ready for layout changes. Using locales=*&stringTest=dynamic&dev It looks like Vertex labels need to re-center and the "ResetShape" button needs to be re-centered upon string change.

jessegreenberg added a commit that referenced this issue Mar 8, 2023
@jessegreenberg
Copy link
Contributor

jessegreenberg commented Mar 8, 2023

Dynamic layout added above. @arouinfar and @terracoda offered to review the behavior during a recent standup meeting, assigning to them and thanks!

EDIT: I also did some testing for memory leaks after adding string Properties and did not see any.

@arouinfar
Copy link

@jessegreenberg I tested this thoroughly with stringTest=dynamic and I'm not seeing any layout issues. Everything looks great!

Also, I just want to note that stringTest=dynamic is a really useful tool. It's much more efficient than cycling through existing locales or customizing the strings in Studio.

My schedule didn't line up with @terracoda, but I shared a link to the documentation in initialize-globals over Slack. I think it's a pretty low barrier to entry, but happy to consult on any future dynamic locale review needs.

Since I don't have any change requests, I think we're good to close.

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

5 participants