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

Apply grey color to disabled toggle slider even if it's checked #5235

Closed
wants to merge 2 commits into from

Conversation

Al2Klimov
Copy link
Member

fixes #5234

Copy link
Contributor

@flourish86 flourish86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your review request. Could you name a view where this is in effect?

It would be helpful if you provided screenshots of before and after versions, so I can directly see the changes. Thanks in advance!

@Al2Klimov
Copy link
Member Author

Of course.

This is the view in question: https://plan.netways.de/icingaweb2/netrp/task/create?user=1

Instead of screenshots, here is even a live view: https://minigame.prod.aklimov.net-dump.de/icingaweb2/fourcolors/demo/

Copy link
Contributor

@flourish86 flourish86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See changes in commit 43a4add

Copy link
Member

@nilmerg nilmerg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was solely about disabled checked sliders. Now it affects also unchecked sliders and destroys their contrast:

Before After
image image
image image

@@ -425,18 +425,12 @@ form.inline {
}

// Disabled inputs

.icinga-controls .toggle-switch.disabled {
.icinga-controls input[type="checkbox"]:disabled + .toggle-switch {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was previously done with the disabled class.

  • Why is this removed?
  • Who set this to begin with?
  • Is it used somewhere still?

@Al2Klimov Al2Klimov assigned flourish86 and unassigned Al2Klimov Aug 8, 2024
@flourish86
Copy link
Contributor

… destroys their contrast:

I basically agree, even if I think "destroy" is a little harsh, but it was intentional. It was more of a quick fix, though.

This was solely about disabled checked sliders. Now it affects also unchecked sliders and …

But only changing the disabled unchecked ones still doesn't solve the main problem. So, here are the main problems I was solving the "before" version:

  • both the disabled ones are standing out way too much. They shouldn't since they're disabled
  • both disabled toggles aren't uniformly looking "disabled" in comparison with "enabled" ones.

So what I did is add another layer for the appearance of the disabled ones, namely opacity. I agree, that especially in direct comparison to each other, this still is far from perfect (especially the light/disabled/unchecked is really low on contrast), but it's still an improvement to before.

What we probably should do to make it as good as possible is to make a refinement of alle the toggles and validate all the versions in a form context. But it would be more efficient to do that in a design tool before implementing it.

@nilmerg
Copy link
Member

nilmerg commented Aug 13, 2024

but it's still an improvement to before.

Nah. It's blurring them so much, that I fail to identify them as toggles if buried between other elements. There's just a dark or light blob. If they keep looking this way, we can simply remove them instead of disabling them, this would have the same effect.

But it would be more efficient to do that in a design tool before implementing it.

Then please do so.

@flourish86
Copy link
Contributor

Nah. It's blurring them so much, that I fail to identify them as toggles if buried between other elements. There's just a dark or light blob. If they keep looking this way, we can simply remove them instead of disabling them, this would have the same effect.

Sorry, but that's just subjective and random bashing without any reasoning behind it à la "I just don't like it". I explained, why it looks that way.

We won't have a final solution without either compromises or completely overhauling the theme (modes). Reducing contrast is an established way to make things disabled.

For Example

https://m2.material.io/components/text-fields#filled-text-field (See section states)

https://react.ui.audi/?path=/story/components-checkbox-all-stories--disabled

https://carbondesignsystem.com/components/toggle/usage/#states

@nilmerg
Copy link
Member

nilmerg commented Aug 14, 2024

Since we don't have a final solution right now, and the suggested changes here don't reflect our final design choices, I'll close here. We'll focus on a proper solution in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disabled and checked checkbox doesn't visually differ from regular checked checkbox
3 participants