Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should all disabled sun components look the same? #665

Closed
zepumph opened this issue Nov 6, 2020 · 2 comments
Closed

Should all disabled sun components look the same? #665

zepumph opened this issue Nov 6, 2020 · 2 comments
Assignees

Comments

@zepumph
Copy link
Member

zepumph commented Nov 6, 2020

There has been a lot of work around factoring out enabled logic from each sun component, and consistently applying visual differences.

Up until this point, the visuals have been this:

      node.opacity = enabled ? 1.0 : options.disabledOpacity;

Where by default, disabledOpacity has been .45.

Previous train of recent enabledProperty work (just the important ones, ordered by time):
#257 (create an EnabledNode mixin)
phetsims/scenery#1100 (move EnabledNode to Node)
phetsims/scenery#1112 (move the above hard coded listener logic into sun from scenery)

Opacity seems to not be the only or preferred strategy for disabling components. Judging from what I heard come out of a meeting in #658 (comment), in general it may be better to not set opacity on the "background" of the component, and potentially only on the content. Additionally, it isn't clear if we in general only apply opacity, or greyscale, or both.

There is also a good chance that we won't get a single strategy for all sun components. Buttons have been updated to use greyscale, which is different from how all other sun components are doing things.

Here are some previous issues where sun component disabled visuals has been discussed.
#578
#171
#478

This issue is to try to decide if there is a sentence that applies to all/most component. Something like "disabled should greyscale the component, and apply opacity to the content", or "disabled should set opacity of the component to .45". It is hard to tell in a lot of usages how much of the deviation is designed, or happen-stance because there wasn't a clear standard when it was being developed.

Note that there will always be the way to override the default for a particular component, see phetsims/scenery#1112 for the implementation of that flexibility.

I don't know how to proceed on this. It feels like outside the scope of the current enabledProperty work. Over to @ariel-phet.

UPDATE: I pulled pickable-related work out to #666.

@arouinfar
Copy link
Contributor

Thanks for opening this issue @zepumph. With the scenery filters, there are a variety of ways we could disable UI components. @jonathanolson, @samreid, and I touched on this a bit when we met to discuss #658 and I definitely think this is something we could discuss further as a team.

@zepumph
Copy link
Member Author

zepumph commented Nov 10, 2020

@ariel-phet and I discussed this, and we feel like in general buttons look better with the greyscale. Even though there are some places where opacity looks better,

Over in phetsims/scenery#1112 we added support for overriding the default enabled strategy, and that could be used to make a button use the "opacity" strategy instead of a greyscale one, like so:

      enabledAppearanceStrategy: ( enabled, background, content ) => {
        options = merge( {
          disabledOpacity: SunConstants.DISABLED_OPACITY
        }, options );

        background.opacity = enabled ? 1.0 : SunConstants.DISABLED_OPACITY;
        content.opacity = enabled ? 1.0 : SunConstants.DISABLED_OPACITY;
      }

Everything besides buttons has worked very well for us using opacity. And a consistent opacity at that (in sun it is .45).

@ariel-phet feels like we should not try to coerce the default button strategy and other sun component strategies to match, but should instead try to treat the general sun component disabled opacity strategy as the default, and realize that buttons are different. See #578 (comment) for proof that this opacity was very thoughtfully designed!

There are some usages in sims and elsewhere that currently use other enabled behavior. When it makes sense, we should try to refactor them to use the same opacity strategy as the default sun component one.

@ariel-phet says that we aren't looking to expand the greyscale strategy to other sun components. There doesn't seem to be a reason to change something that has been working (has been interviewed on/production tested/etc).

@zepumph zepumph closed this as completed Nov 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants