There should be a way within the sim to set the default temperature units

jbphet commented 2 years ago

Currently the sim defaults to Kelvin, and we should probably have a way within the sim to set the default units to something else, and that setting should still be respected after a reset. Issue #72 will be related.

jbphet commented 2 years ago

At this point, we're not sure if this will be in an "Options..." dialog or a "Preferences" dialog. It may hinge upon whether we have extra sounds. I'll defer this for now, and once things are further along we can add the appropriate item to the appropriate dialog.

jbphet commented 1 year ago

This was discussed in the 12/14/2022 meeting, and we decided that yes, it would be good to have this. It is not blocking for the publication of the prototype for #220. Undeferring and assigning to myself.

jbphet commented 1 year ago

This has now been implemented. @arouinfar - please review the behavior on master and see if you approve. Please assign back to me when done, since I'd like to leave it open and have QA check it on the next release.

I have two specific questions about this feature:

[ ] I made the units change in the sim when the selected default is changed from the Preferences menu. It felt weird to me when it didn't to this, but it could lead to a slightly odd scenario where the user intentionally changes to one units value in a screen, then changes the default via the preferences menu to something different, and their previous intentional non-default selection changes. This seems like an unlikely use case, and not particularly surprising even if it does occur, so I opted for changing the units selection everywhere when the default changes. Are you good with this?
[ ] Do you think we should make the query parameter for the default units tolerant of lower case values? For example, should it accept "k" as well as "K"?

The tab in the "Preferences" dialog where this can be set looks like this:

arouinfar commented 1 year ago

please review the behavior on master and see if you approve.

Behavior on master looks good, but the casing of some of the tandem names looks off. Can you please use camel casing for the name of the control defaultTemperatureUnitsSelector as well as the individual radio buttons, e.g. celsiusRadioButton?

I made the units change in the sim when the selected default is changed from the Preferences menu. It felt weird to me when it didn't to this

This feels really natural to me. If someone is taking the time to change the default temperature units in the Preferences dialog, it's not a huge leap to assume they prefer to work in that unit.

Do you think we should make the query parameter for the default units tolerant of lower case values? For example, should it accept "k" as well as "K"?

Currently, the accepted values are uppercase which naturally corresponds to how the units are displayed in the sim. This seemingly violates the norms of query parameters (all camel casing). I think it would be a nice touch to accept both k and K for Kelvin, but I probably would only advertise the lowercase version since that follows the pattern used elsewhere across sims.

arouinfar commented 1 year ago

While reviewing dynamic layout, I noticed the Preferences dialog can get really wide. It seems like the Default Temperature Units control may need a narrower maxWidth.

The content in the other tabs is width-limited, so I think the Simulation tab should match. Here's the Visual tab for comparison.

pixelzoom commented 1 year ago

maxWidth was not set for "Default Temperature Units". That has been fixed in the above commit. @arouinfar please review in master, close if OK.

arouinfar commented 1 year ago

Thanks @pixelzoom the maxWidth looks good in master. There are some additional comments for @jbphet in https://github.com/phetsims/greenhouse-effect/issues/187#issuecomment-1585220035, so I'll reassign back to him.

pixelzoom commented 1 year ago

... There are some additional comments for @jbphet in https://github.com/phetsims/greenhouse-effect/issues/187#issuecomment-1585220035, so I'll reassign back to him.

From https://github.com/phetsims/greenhouse-effect/issues/187#issuecomment-1585220035:

Behavior on master looks good, but the casing of some of the tandem names looks off. Can you please use camel casing for the name of the control defaultTemperatureUnitsSelector as well as the individual radio buttons, e.g. celsiusRadioButton?

Done in the above commit. Here's the Studio tree:

I think it would be a nice touch to accept both k and K for Kelvin, but I probably would only advertise the lowercase version since that follows the pattern used elsewhere across sims.

The defaultTemperatureUnits query parameter currently accepts value 'K', 'C', 'F'. There is no requirement/convention that query parameter values need to begin with a lowercase letter. Uppercase feels correct/natural to me here -- it's what I would try without consulting documentation. I don't think it needs to support both uppercase and lowercase, and I don't think that supporting lowercase only would be an improvement.

Back to @arouinfar to review. Let me know if I missed anything else in this issue.

arouinfar commented 1 year ago

There is no requirement/convention that query parameter values need to begin with a lowercase letter.

I didn't realize this, thanks for letting me know @pixelzoom. In that case, let's continue with just the uppercase values since they are the more natural choice.

Everything looks good, closing.

phetsims / greenhouse-effect

There should be a way within the sim to set the default temperature units #187