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

fix(switch): add text color for switch toggle #2309

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jwueller
Copy link

@jwueller jwueller commented Apr 4, 2024

Introduces a --_on-background variable that's used to define the IO text color relative to the --_background color of the toggle, and applies it to the base state.

Note that this doesn't apply to the active state, beacuse that already uses an explicit text color for the IO text.

The Problem

Black background with white text; both the label and IO text have poor to no contrast.
grafik

This can be mitigated by setting --color-label to currentColor to honor the chosen text color. Maybe this should even be the default, considering the label is always going to be on top of the surrounding background? The contrast problem remains for the IO text, and is not adjustable by the user.
grafik

The Solution

Since the switch toggle itself already has a background color, contrast becomes independent of the surrounding background. It's therefore inconsistent to honor --color-label in this context. The new variable ensures that the base states color actually related to the background it's drawn on.
grafik

It currently defaults to var(--telekom-color-text-and-icon-standard), which also behaves correctly when the control is used in dark mode.
grafik

The switch toggle itself already has a background color, which means
that the IO label on top of it should be independent of the surrounding
--color-label to ensure proper contrast when the toggle background and
the label background differ significantly.
Copy link

netlify bot commented Apr 4, 2024

Deploy Preview for marvelous-moxie-a6e2fe ready!

Name Link
🔨 Latest commit 590050f
🔍 Latest deploy log https://app.netlify.com/sites/marvelous-moxie-a6e2fe/deploys/660ee7d9c0c1440008d0e7b7
😎 Deploy Preview https://deploy-preview-2309--marvelous-moxie-a6e2fe.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Collaborator

@maomaoZH maomaoZH left a comment

Choose a reason for hiding this comment

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

@jwueller Thanks for the PR. LGTM!

@amir-ba amir-ba added bug Something isn't working accessibility Related to accessibility labels Aug 28, 2024
@tshimber
Copy link
Contributor

@amir-ba i guess can be merged

@amir-ba amir-ba self-requested a review October 23, 2024 12:39
Copy link
Collaborator

@amir-ba amir-ba left a comment

Choose a reason for hiding this comment

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

@jwueller thanks for this, and sorry for the late response. Can you please merge the main branch into this, so the checks can run correctly again. We will merge this afterward

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility Related to accessibility bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants