search

phetsims / joist

Joist is the main framework for PhET Interactive Simulations. Joist creates and displays the simulation content, home screen, navigation bar, About dialog, enables switching between tabs, and other framework-related features.
MIT License
8 stars 6 forks source link

Consolidate PreferencesConfiguration and simFeatures #834

Closed jessegreenberg closed 2 years ago

jessegreenberg commented 2 years ago

We used to have simFeatures as the only entry point to enable or disable features for a PhET sim. Now we have PreferencesConfiguration which is also a way to enable other features and their items in the Preferences Dialog. They have overlap, and it is confusing which should take priority. I noticed this because I tried to enable Voicing from PreferencesConfiguration but found it did nothing because Voicing has to be enabled from simFeatures in package.json.

We should look at these two and decide how to consolidate.

jessegreenberg commented 2 years ago

Its likely to support this we will need a default PreferencesConfiguration, which is a step in the right direction. It is strange to require an empty PreferencesConfiguration just to see the Preferences Dialog.

zepumph commented 2 years ago

@jessegreenberg and I were working on this today. We decided that simFeatures probably need to stick around so that we can still have data lists, but it wasn't clear to us if the query parameters should override the options passed when creating a PreferencesModel.

Up next we are going to combine PreferencesModel and PreferencesConfiguration. This is only possible because we can now have an empty default.

zepumph commented 2 years ago

We were able to combine these two and delete PreferencesConfiguration. We decided that we didn't want to support query parameters overriding the value passed into the preferencesModel at this time. It seemed fine since most options should come from simFeatures anyways. @jessegreenberg ready to close?

jessegreenberg commented 2 years ago

Thanks! There was one more TODO about shouldShowDialog, addressed in the above commit.

We decided that we didn't want to support query parameters overriding the value passed into the preferencesModel at this time.

I removed the support for supportsMultipleLocales for this so it isn't a special case. Now I don't see a need to keep ?supportsMultipleLocales query parameter so I removed it.

I did a second scan through above commits and things look good. I think this can be closed if these changes are OK with you.