Skip to content
New issue

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

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

Already on GitHub? # to your account

fix(a11y): update vertical nav selected and hover colors #1491

Merged
merged 7 commits into from
Aug 2, 2024

Conversation

andyfeds
Copy link
Contributor

@andyfeds andyfeds commented Jul 25, 2024

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • If applicable, have a visual design approval

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Issue Number: CDE-2115

What is the new behavior?

To fix the a11y defect the below changes have been made:

  • Keep current unselected background colors.
  • Update the selected option background to white.
  • Hover and active (clicked) colors to be different from one another for light theme. Hover (construction-200). Active (construction-300)

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Copy link
Contributor

github-actions bot commented Jul 25, 2024

👋 @andyfeds,

  • 🙏 The Clarity team thanks you for opening a pull request
  • 🎉 The build for this PR has succeeded
  • 🔍 The PR is now ready for review
  • 🍿 In the meantime, view a preview of this PR
  • 🖐 You can always follow up here. If you're a VMware employee, you can also reach us on our internal Clarity Support space

Thank you,

🤖 Clarity Release Bot

@andyfeds andyfeds self-assigned this Jul 25, 2024
Copy link
Contributor

This PR introduces visual changes: 6cfb203
If these changes are intended and correct, please cherry-pick the above commit to this PR.

git checkout a11y/CDE-2115
git fetch https://github.com/vmware-clarity/ng-clarity.git 6cfb20359f0164c0a3e20f88eeb3d96080d103f3
git cherry-pick 6cfb20359f0164c0a3e20f88eeb3d96080d103f3
git push

Copy link
Contributor

This PR introduces visual changes: c6159c1
If these changes are intended and correct, please cherry-pick the above commit to this PR.

git checkout a11y/CDE-2115
git fetch https://github.com/vmware-clarity/ng-clarity.git c6159c1e4493989663c92933754643b963b568a9
git cherry-pick c6159c1e4493989663c92933754643b963b568a9
git push

@andyfeds andyfeds requested a review from a team July 25, 2024 13:29
@kevinbuhmann kevinbuhmann changed the title fix(a11y): updated vertical nav selected and hover colors fix(a11y): update vertical nav selected and hover colors Jul 25, 2024
Copy link
Contributor

@valentin-mladenov valentin-mladenov left a comment

Choose a reason for hiding this comment

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

I have noticed that in light theme --clr-vertical-nav-hover-bg-color and --clr-vertical-nav-active-bg-color are using the same construction-100 color.

@andyfeds
Copy link
Contributor Author

I have noticed that in light theme --clr-vertical-nav-hover-bg-color and --clr-vertical-nav-active-bg-color are using the same construction-100 color.

Yes, I see that too, will confirm this with Grigor

Copy link
Contributor

@Jinnie Jinnie left a comment

Choose a reason for hiding this comment

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

Assigning global colors to --clr-... tokens is a bit controversial.
It requires dark-theme overrides in the component specific _properties.vertical-nav.scss file, which short-circuits any possibility to deliver proper theming through design tokens.
I think we should stick to using --cds-alias-... aliases as immediate assignment to the --clr-... space tokens, and not directly link to global colors.

@andyfeds andyfeds force-pushed the a11y/CDE-2115 branch 4 times, most recently from 7bffacf to ee12bfd Compare July 29, 2024 10:26
@andyfeds
Copy link
Contributor Author

The new cds-alias tokens for the vertical nav have to be added in @cds/core which involves changes to the core version.
As a temporary solution, I've added --cds-alias-... in the temp-overrides.scss file for now and mapped these to the --clr-... space tokens instead of the global variables. Let me know if you see any issues @Jinnie @valentin-mladenov

Copy link
Contributor

This PR introduces visual changes: 5b96551
If these changes are intended and correct, please cherry-pick the above commit to this PR.

git checkout a11y/CDE-2115
git fetch https://github.com/vmware-clarity/ng-clarity.git 5b96551eb58324b96bb297181cedd8091d054ca2
git cherry-pick 5b96551eb58324b96bb297181cedd8091d054ca2
git push

@valentin-mladenov valentin-mladenov self-requested a review July 30, 2024 12:26
Copy link
Contributor

@valentin-mladenov valentin-mladenov left a comment

Choose a reason for hiding this comment

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

LGTM

@andyfeds andyfeds merged commit 631d2ff into vmware-clarity:main Aug 2, 2024
8 checks passed
Copy link
Contributor

Hi there 👋, this is an automated message. To help Clarity keep track of discussions, we automatically lock closed PRs after 14 days. Please look for another open issue or open a new issue with updated details and reference this one as necessary.

@github-actions github-actions bot locked and limited conversation to collaborators Aug 17, 2024
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants