phetsims / number-compare

"Number Compare" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
0 stars 0 forks source link

Insufficent space for the LocaleSwitch in the Compare screen #18

Closed pixelzoom closed 1 year ago

pixelzoom commented 1 year ago

There is insufficent space for the LocaleSwitch in the Compare screen. To reproduce:

  1. Run the sim with locales=*
  2. Go to the Compare screen
  3. Go to Preferences dialog, choose Simulation tab.
  4. Turn on "Second Language" toggle, choose "español (España)"
  5. Close the Preferences dialog.
  6. Note that the LocaleSwitch is really compromised. The "español (España)" is in tiny font, and it's kissing the accordion box to its right. Here's a screenshot:
  7. screenshot_2229

This feels like a fundamental design flaw -- there simply is not enough space for this control. Since this compromises the UX, I think it needs to be resolved for publication.

pixelzoom commented 1 year ago

Here's another example when running with locale=es_MX&locales=*. I doubt that our Mexican users will be happy with this UX.

screenshot_2230
pixelzoom commented 1 year ago

The most expedient fix woud be to move the toggle switch down, and increase the maxWidth for its labels. See first screenshot below.

I realize that you probably placed it in proximilty to the SpeechSynthesisButton. But the toggle also lacks proximity to that button in Number Play, so you'd be consistent :) See second screenshot below.

nc

screenshot_2235
amanda-phet commented 1 year ago

The location you proposed looks great to me! It's in line with the sentence that it's translating, so I think it works.

pixelzoom commented 1 year ago

Ugh, we still have a space problem here. With stringTest=long, you can see that the "sentence" grows to overlap the toggle. Screenshot below. I'm hesitant to change the maxWidth for the sentence, because I suspect that it was purposely set to this width, and the text already seems a bit too small to me (even in English).

@amanda-phet thoughts?

screenshot_2238
pixelzoom commented 1 year ago

Brainstorming... I was going to suggest using radio buttons instead of a toggle switch for this sim. But I don't like the fact that it would be a different control than in the Number Play sim. And there would still have a space problem, see mockup below.

screenshot_2241
amanda-phet commented 1 year ago

I don't know what to do!

Here is a wild idea. Instead of a max width for the sentence, can we have a max width while center-justified, and if it reaches that max width, it will change to become left-justified? So in the case of a super long string, the string would actually not look centered anymore, and would use that space to the right of where the long string currently is?

image

amanda-phet commented 1 year ago

Discussed this 2/6/23 and decided to try radio buttons. @pixelzoom will take it from here.

pixelzoom commented 1 year ago

More notes from 2/6/23 design meeting with @amanda-phet @chrisklus @catherinecarter @marlitas @pixelzoom

We identified this configuration as a particularly long sentence,?locales=*&locale=st&secondLocale=en_GB:

screenshot_2277

We considered shifting the sentence to the right (@amanda-phet's suggesting in https://github.com/phetsims/number-compare/issues/18#issuecomment-1416412393) and decided that this would not be a good UX, and would be a complicated layout.

We considered moving buttons around, so that we could move the toggle more toward the left edge of the layoutBounds. But we didn't like that layout.

@amanda-phet suggested moving the toggle to the right side of the ScreenView. But there's really no space there, and the toggle would no longer be in proximity to the speech button.

@amanda-phet asked if we really needed long locale names like "English (United Kingdom)". Those are standard names, and not something that we can consider changing.

We finally decided that using radio buttons would be the lesser of all evils. We don't like the inconsistency of UI between the 2 sims, and there's an additional (duplicate) development cost. But we can't figure out how to make the same control work in both sims. @pixelzoom will implement radio buttons.

pixelzoom commented 1 year ago

@chrisklus @amanda-phet please review. There are 3 significant changes in the above commits.

(1) Locale radio buttons are now used in Number Compare. The implementation is LocaleRadioButtonGroup. Here are some screenshots:

screenshot_2278 screenshot_2279 screenshot_2280

(2) LocaleSwitch was changed to pass a DerivedProperty to Text, so that it matches the implementation of LocaleRadioButtonGroup. This is the modern approach to supporting dynamic strings. @chrisklus FYI.

(3) Changed locale switch to be left-justified in Number Play. (The toggle switch was previously centered below the accordion box.) I reviewed this with @chrisklus, and we agreed that it looks better and improves proximity to the speech buttons. Screenshots:

screenshot_2275 screenshot_2276
amanda-phet commented 1 year ago

(1) There is a bug where the radio buttons show up all the time, regardless of the preference being on.

(3) This looks fine to me. I like that there is more room for either locale to fit.

chrisklus commented 1 year ago

For (3), maybe this is complex, but it would be cool if the whole control had a max width and the labels knew how to scale equally if needed. Then in the screenshot in https://github.com/phetsims/number-compare/issues/18#issuecomment-1419575077, "English (United Kingdom)" would not need to be scaled down as it is right now.

pixelzoom commented 1 year ago

(1) There is a bug where the radio buttons show up all the time, regardless of the preference being on.

Fixed in https://github.com/phetsims/number-compare/commit/89e730e9011d0c924b369e667300e8c77e7db887.

For (3), maybe this is complex, but it would be cool if the whole control had a max width and the labels knew how to scale equally if needed. ...

Done in https://github.com/phetsims/number-suite-common/commit/04b27b147e950ff6ab88aac4cc926b1423dcfea7. See LocaleSwitch.ts line 59-73.

chrisklus commented 1 year ago

Awesome @pixelzoom, thanks! @marlitas and I reviewed all of the commits and tested the behavior and we like these changes. With @amanda-phet's review complete in https://github.com/phetsims/number-compare/issues/18#issuecomment-1419599763, I think we are all set to close here.

pixelzoom commented 1 year ago

Reopening. I'm suddenly seeing bad maxWidth for the LocaleSwitch labels in master, see below. Did something get reverted?

screenshot_2287
pixelzoom commented 1 year ago

I moved the problem reported in https://github.com/phetsims/number-compare/issues/18#issuecomment-1421212885 to https://github.com/phetsims/number-play/issues/210. So re-closing.