phetsims / sun

User-interface components for PhET simulations, built on top of Scenery.
MIT License
4 stars 12 forks source link

ABSwitch `enabledProperty: false` appearance needs improvment #853

Closed arouinfar closed 11 months ago

arouinfar commented 11 months ago

Discovered in https://github.com/phetsims/acid-base-solutions/issues/190, but decided with @pixelzoom that this does not block publication of Acid-Base Solutions with PhET-iO. Previous discussion of the PhET-iO design of ABSwitch and ToggleSwitch can be found in https://github.com/phetsims/sun/issues/571.

Here's an example of an ABSwitch is Acid-Base Solutions. The non-selected Node has reduced opacity specified by SceneryConstants.DISABLED_OPACITY.

image

If a client wants to disable interaction with the weakStrongSwitch, they can set its enabledProperty to false. This results in the non-selected Node appearing doubly disabled, which doesn't look great. In this context, it may be important to clients to emphasize that the acid/base in the beaker is "Weak", so hiding the ABSwitch isn't the solution either.

image

Disabling the ToggleSwitch has the ideal appearance. The control looks disabled while maintaining readability. However, the ABSwitich is still interactive, as the A and B Nodes remain pickable. Students can click on "strong" to change the switch state.

image

It would be really nice if ABSwitch could retain this appearance when its enabledProperty is false (ToggleSwitch disabled, A & B Nodes non-pickable).

As an aside, there is a larger issue related to "display" modes of UI components in https://github.com/phetsims/phet-io/issues/1757. I think ABSwitch would benefit from a displayOnlyProperty, but I think this issue with enabledProperty is distinctly different as it impacts phet-brand too. I've framed this issue as a PhET-iO design issue, but it's not. However, this issue is more relevant in phet-io-brand where clients have control over the enabledProperty.

While we deemed this was non-blocking for Acid-Base Solutions, I think this is worthy of being triaged and considered for a future iteration (or perhaps as a larger part of PhET-iO maintenance). I'll tag this for PhET-iO meeting to discuss.

zepumph commented 11 months ago

Discussed during phet-io meeting today. It seems like a reasonable change that may not open too many cans o worms (fingers crossed). We discussed that the primary way to get something like this (nice but not high priority or super important) completed would probably be to attach it to a sim's work, like acid base solutions.

@samreid and I also mentioned that in the third picture, it is a bit weird to have "weak" not be greyed out. Perhaps both labels and the switch can all get the same, single enabled layer onto them.

zepumph commented 11 months ago

@kathy-phet recommends seeing if the acid base solutions team wants to bring this into that sim.

arouinfar commented 11 months ago

Discussed with @pixelzoom and we decided to move forward to uniformly applying SceneryConstants.DISABLED_OPACITY to the entire ABSwitch when disabled. This way the unselected Node won't appear doubly grayed out.

pixelzoom commented 11 months ago

@arouinfar This is ready for your review. Close if OK.

To verifiy, observe the change in the UI when:

arouinfar commented 11 months ago

Thanks @pixelzoom. I tested using Acid-Base Solutions in Studio as recommended, and the behavior looks great. I think this is definitely an improvement. Closing.

Here's what a disabled ABSwitch now looks like:

image