phetsims / natural-selection

"Natural Selection" is an educational simulation in HTML5, by PhET Interactive Simulations
GNU General Public License v3.0
3 stars 7 forks source link

Add support for dynamic locale #319

Closed pixelzoom closed 1 year ago

pixelzoom commented 2 years ago

@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:

@amanda-phet @kathy-phet FYI.

pixelzoom commented 2 years ago

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

pixelzoom commented 1 year ago

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 https://github.com/phetsims/chipper/issues/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 commented 1 year ago

TODO:

pixelzoom commented 1 year ago

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 https://github.com/phetsims/chipper/issues/1314 and https://github.com/phetsims/sun/issues/746.

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

pixelzoom commented 1 year ago

Notes from 9/1/2022 design meeting:

arouinfar commented 1 year ago

@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 commented 1 year ago

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

arouinfar commented 1 year ago

Looks good on master, thanks @pixelzoom. Closing.