-
Notifications
You must be signed in to change notification settings - Fork 0
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
Why is "Region and Culture" in PDL and PSD? #324
Comments
Please note that the tree referred to above is only in the strings, the full tandem is:
There are also other irrelevant strings, such as voicing (the sim does not support voicing)
This problem was previously identified and discussed in https://github.com/phetsims/phet-io/issues/1877. I also confirmed that moving the string accessors from static to the constructor does not eliminate them from the API: Subject: [PATCH] During migration schema compatibility testing, if one has parameterValues and the other does not, they are not compatible. undefined is not compatible with the empty array[], see https://github.com/phetsims/density/issues/196
---
Index: js/preferences/RegionAndCultureComboBox.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/preferences/RegionAndCultureComboBox.ts b/js/preferences/RegionAndCultureComboBox.ts
--- a/js/preferences/RegionAndCultureComboBox.ts (revision b94d701c92cab77cd4cf554f333a7e0701c5b01b)
+++ b/js/preferences/RegionAndCultureComboBox.ts (date 1714736472872)
@@ -17,24 +17,24 @@
import JoistStrings from '../JoistStrings.js';
import PickOptional from '../../../phet-core/js/types/PickOptional.js';
-// Maps a RegionAndCulture value to a StringProperty.
-const STRING_PROPERTY_MAP: Record<RegionAndCulture, LocalizedStringProperty> = {
- africa: JoistStrings.preferences.tabs.localization.regionAndCulture.africaStringProperty,
- africaModest: JoistStrings.preferences.tabs.localization.regionAndCulture.africaModestStringProperty,
- asia: JoistStrings.preferences.tabs.localization.regionAndCulture.asiaStringProperty,
- latinAmerica: JoistStrings.preferences.tabs.localization.regionAndCulture.latinAmericaStringProperty,
- oceania: JoistStrings.preferences.tabs.localization.regionAndCulture.oceaniaStringProperty,
- random: JoistStrings.preferences.tabs.localization.regionAndCulture.randomStringProperty,
- usa: JoistStrings.preferences.tabs.localization.regionAndCulture.unitedStatesOfAmericaStringProperty
-};
+type SelfOptions = EmptySelfOptions;
+type RegionAndCultureComboBoxOptions = SelfOptions & PickOptional<ComboBoxOptions, 'tandem'>;
+
+class RegionAndCultureComboBox extends ComboBox<RegionAndCulture> {
+
+ public constructor( providedOptions?: RegionAndCultureComboBoxOptions ) {
+
+ // Maps a RegionAndCulture value to a StringProperty.
+ const STRING_PROPERTY_MAP: Record<RegionAndCulture, LocalizedStringProperty> = {
+ africa: JoistStrings.preferences.tabs.localization.regionAndCulture.africaStringProperty,
+ africaModest: JoistStrings.preferences.tabs.localization.regionAndCulture.africaModestStringProperty,
+ asia: JoistStrings.preferences.tabs.localization.regionAndCulture.asiaStringProperty,
+ latinAmerica: JoistStrings.preferences.tabs.localization.regionAndCulture.latinAmericaStringProperty,
+ oceania: JoistStrings.preferences.tabs.localization.regionAndCulture.oceaniaStringProperty,
+ random: JoistStrings.preferences.tabs.localization.regionAndCulture.randomStringProperty,
+ usa: JoistStrings.preferences.tabs.localization.regionAndCulture.unitedStatesOfAmericaStringProperty
+ };
-type SelfOptions = EmptySelfOptions;
-type RegionAndCultureComboBoxOptions = SelfOptions & PickOptional<ComboBoxOptions, 'tandem'>;
-
-class RegionAndCultureComboBox extends ComboBox<RegionAndCulture> {
-
- public constructor( providedOptions?: RegionAndCultureComboBoxOptions ) {
-
const options = optionize<RegionAndCultureComboBoxOptions, SelfOptions, ComboBoxOptions>()( {
// For now, do not instrument Preferences elements, see https://github.com/phetsims/joist/issues/744#issuecomment-1196028362
Also, if those strings appear in another simulation that actually does support R&C, then later undergoes change, whatever migration rules we introduce in that sim can be reused here. I'm not saying it would be 0 overhead, but it could be shared overhead.
I do not see the string supportsRegionAndCulture in the API json: https://phet-dev.colorado.edu/html/projectile-data-lab/1.0.0-rc.5/phet-io/projectile-data-lab-phet-io-api.json, were you referring to a different JSON? It is also not in the package.json: projectile-data-lab/package.json Lines 25 to 29 in cee42cc
Since November 2023 all new sims have this commit phetsims/chipper@dd94297 which means all sims since then support the region and culture query parameter key. But each sim only support the query parameter values that it lists in the sim package.json, or defaults to
I'm interested to hear more about this, was it limited to strings or about more than strings? A note for other team members as we look into this: "unbuilt" mode shows more irrelevant strings than "built" mode does, so please be aware of that. |
I'm not worried about the unused Strings. I'm considering the case - similar to locale - where a client appends regionAndCulture=africa to query parameters because that is their preferred regionAndCulture for their product. This sim would pop up a dialog, even though it basically is unaffected by regionAndCulture at all and is never planned to be. Where as locale=es will just continue, even if es is not an option. Is there such a thing as |
Idea for discussion, populate the schema conditionally and dynamically (untested): /*
* Sets the default for the Region and Culture feature. The set of valid values is determined by
* "supportedRegionsAndCulturesValues" in package.json. If not provided in the URL, the default can
* be set via "defaultRegionAndCulture" in package.json, which defaults to 'usa'.
*/
regionAndCulture: ( packagePhet?.simFeatures?.supportedRegionsAndCultures?.length > 0 ) ? {
public: true,
type: 'string',
defaultValue: packagePhet?.simFeatures?.defaultRegionAndCulture ?? 'usa',
validValues: packagePhet?.simFeatures?.supportedRegionsAndCultures ?? [ 'usa' ] // default value must be in validValues
} : {
public: false,
type: 'string',
defaultValue: 'usa',
isValidValue: () => true
}, |
Related issue where we most recently discussed locale fallbacks and when error message popups should appear: phetsims/joist#963 (comment) |
@kathy-phet @samreid @matthew-blackman I don't think this is a blocking issue for PDL/PSD, as @samreid points out all sims published after November 2023 support the Any changes that arise from this issue would also apply to a handful of recently published phet-brand and phet-io-brand sims.
Thanks @samreid, this was really helpful to know.
A good reminder @samreid! However, note that all Preferences strings will appear in a built PhET-iO sim, even for irrelevant Preferences. Other irrelevant common code strings (e.g. such as those under Like @kathy-phet I am not concerned with
@kathy-phet this sounds like a client training issue. All sims support
We do this with lots of other features/Preferences, but you can always set them to I'm not worried about clients doing this, but I bring it up because I would expect it to behave like the other "supports" options in initialize-globals.js. |
@samreid, @matthew-blackman, @brent-phet and I met to talk through some of these questions this morning. We looked through other query parameter examples and how they behave if the sim doesn't specifically support them. For example, we tried setting Either way I don't believe it's blocking as a change would require a maintenance release. |
Noting that 2 PhET-iO sims currently have ?regionAndCulture query parameter behavior: I do think that we should revisit the use case for regionAndCulture, which is more parallel with locale than with other sim-specific features (even though it is sim specific). I was mostly concerned with bringing this behavior into PhET-iO sims that don't need have a regionAndCulture aspect, but it's already in 2 PhET-iO sims. I'll put a discussion note on PhET-iO meetings. And I'll mark this as deferred at the moment. Not blocking publication. |
@matthew-blackman @samreid @arouinfar - Since this sim doesn't have any regionAndCulture features, why do we have "supportsRegionAndCulture" true in the json, and thus bringing in the ?regionAndCulture query parameter behavior?
Shouldn't that just not be in the json, so we don't complicate this sim with those query parameters at this point?
Since RegionAndCulture is new behavior AND this is a PHET-iO sim for which we don't have RegionAndCulture, we haven't tested it fully and I don't know that this is the sim to adopt this into the API on? I hadn't realized RegionAndCulture would be active on non RegionAndCulture related sims until I ran into it with Gas Properties yesterday.
I want to make sure someone has decided that yes, we want to have this part of the tree in here??
And yes, we want the sim to say it an unsupported value if regionAndCulture=africa is added, because technically there is no special values.
Should
supportsRegionsAndCultures
just not be in the json for this sim?The text was updated successfully, but these errors were encountered: