phetsims/scenery-phet

Convert scenery-phet string usage to dynamic strings

Closed this issue · 9 comments

jonathanolson commented
Convert scenery-phet string usage to dynamic strings
pixelzoom commented

I'll start working on this.

As noted in 10/24/2022 sprint meeting, I'll probably skip SceneryPhetStrings.a11y, because some of the a11y code is still .js, and I'm not familiar with how to test it.

zepumph commented

I converted all usages of SceneryPhetStrings outside of scenery-phet to be a StringProperty.

  • 🎉1
pixelzoom commented

@zepumph said:

I converted all usages of SceneryPhetStrings outside of scenery-phet to be a StringProperty.

Just about everywhere this was done, I see this sort of naming problem, e.g. in GFLBVoicingSummaryDescriber.ts:

const singleScreenIntroPatternString = SceneryPhetStrings.a11y.voicing.simSection.screenSummary.singleScreenIntroPatternStringProperty;

The const is a Property, so it should have a "Property" suffix, e.g. singleScreenIntroPatternStringProperty.

I have a fix coming shortly.

pixelzoom commented

"Property" suffix added where appropriate in the above commits. @zepumph feel free to take a peek.

  • 👍1
pixelzoom commented

@zepumph could you please review ed2a10e? My understanding is that labelContent supports type string | TReadOnlyProperty<string>. So labelString in SpeedMap entries is unnecessary, redundant, and undesirable, and we can use stringProperty.

  • 👍1
pixelzoom commented

There are still static strings in 2 files:

  • ScreenSummaryNode.ts
  • SliderControlsKeyboardHelpSection.ts
pixelzoom commented

package.json now contains "supportsInteractiveDescription": true,, and SceneryPhetStrings.ts is exclusively LinkableProperty<string>. So I think this issue is done.

@zepumph anything you'd like to add? Feel free to close.

zepumph commented

I searched for StringProperty = new DerivedProperty and found a case where I felt like PatternStringProperty was simpler and easier. All the other usages seemed right to use DerivedProperty because of extra logic. If we converted them to PatternStringProperty, it would require mapping a value into a new DerivedProperty instance to pass that to the PatternStringProperty which seems annoying and dumb. Ready to close?

pixelzoom commented

👍🏻 closing