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

Problems with sorting the list for RegionAndCultureComboBox #955

Closed pixelzoom closed 6 months ago

pixelzoom commented 7 months ago

In RegionAndCultureComboBox, we have:

    // Sort the region and culture choices, with the default (initially loaded) at the top.
    const localeCompare = new Intl.Collator( phet.chipper.locale ).compare;
    const comboBoxItems = supportedRegionAndCultureValues.sort( ... ).map( ... );

Three problems with the sorting implementation:

(1) Not sure if this is really sorting correctly for non-English locales. Intl.Collator does "language-specific string comparisons", but has it been tested? (2) If the locale is changed, the list is not be resorted. ComboBox has no API for this. (3) Putting the default value at the top of the list is odd, and we generally do not do this, especially when when sorting. If you want to come back to the default value, it can actually be more difficult since that value is not in the sorted order.

@jessegreenberg @marlitas @jonathanolson Do you recall the history here? And reason why we should not correct these things?

I'm also wondering how (or whether) this same issue is addressed for the Locale panel.

jonathanolson commented 7 months ago

I added this, but I messed up (1), and knew about (2).

(3) I believe was specifically requested. It seems easier to have a consistent sort.

It's my opinion that if we are sorting things alphabetically, we should do that with the proper sort for the locale we are using. Thoughts about fixing (1) and (2)?

jonathanolson commented 7 months ago

Ahh, the reason I didn't do (2) is due to the phet-io cost. I'd need to either use MutableOptionsNode on the ComboBox to wipe it when we need a new order, or (the preferable option) implement mutable orders for ComboBox. That second option should probably be done eventually.

jessegreenberg commented 7 months ago

Sorry, I don't know. It looks like sorting was all added recently for https://github.com/phetsims/joist/issues/953 from 82a9de35e1707cdc76bd26e7c25ca21df65c8e1d.

I'm also wondering how (or whether) this same issue is addressed for the Locale panel.

The LocalePanel uses this sorted array https://github.com/phetsims/joist/blob/55930fbd1787868915d895e23645a2afedd9bc4e/js/i18n/localeProperty.ts#L21-L24

marlitas commented 7 months ago

I believe default at the top was just what was initially used because of how the default would be the first element in the package.json array. I have no qualms staying away from that, but it is a little bit of a bummer that some sims are already published with the default at the top behavior.

pixelzoom commented 7 months ago

To be discussed in 3/7/2024 design meeting.

pixelzoom commented 6 months ago

From 3/7/2024 design meeting with @amanda-phet @kathy-phet @Luisav1 @jbphet @jonathanolson. FYI @marlitas.

We decided to make this simple and use one consist ordering of the listbox, regardless of which value is initially selected:

The order is:

Africa Africa (Modest) Asia Latin America Oceania Random USA

We will not attempt to sort these by translated string, or re-sort them when locale is changed. This should be fine since there are a relatively small number of values.

I'll handle implementation.

pixelzoom commented 6 months ago

I discovered an additional problem. sort modifies the array in place, so this is actually changing supportedRegionAndCultureValues.

const comboBoxItems = supportedRegionAndCultureValues.sort( ... ).map( ... );
pixelzoom commented 6 months ago

In https://github.com/phetsims/joist/commit/4d313b7adba6aa22f7c70abad1606bdb5c5446cc:

pixelzoom commented 6 months ago

I've done the work here, but will wait to have this reviewed until https://github.com/phetsims/joist/issues/954 is completed.

pixelzoom commented 6 months ago

@amanda-phet ready for review. The Region and Culture combo box will always be in this order:

Africa Africa (Modest) Asia Latin America Oceania Random USA

amanda-phet commented 6 months ago

That sort order is fine.