Skip to content
This repository has been archived by the owner on Jan 3, 2024. It is now read-only.

A11y fix sufficient text contrast #1744

Merged
merged 2 commits into from
Jan 25, 2022
Merged

A11y fix sufficient text contrast #1744

merged 2 commits into from
Jan 25, 2022

Conversation

stevefrenzel
Copy link
Contributor

@stevefrenzel stevefrenzel commented Oct 29, 2021 •
edited by pascalwengerter
Loading

Description

Higher contrast ratio for certain links and buttons.

Related Issue

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation added/updated

@stevefrenzel stevefrenzel self-assigned this Oct 29, 2021
@update-docs
Copy link

update-docs bot commented Oct 29, 2021

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

pascalwengerter
pascalwengerter suggested changes Nov 3, 2021
View reviewed changes
src/components/__snapshots__/OcModal.spec.js.snap Outdated Show resolved Hide resolved
src/components/OcBreadcrumb.vue Outdated
}

> li a:hover,
> li span:hover,
> li button:hover {
color: var(--oc-color-swatch-primary-default);
color: var(--oc-color-text-default);
text-decoration: underline;
Copy link
Contributor

Choose a reason for hiding this comment

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

This would also set a hover for a span element. Not sure if we want this, what was the idea behind it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was already implemented, I just changed the color reference to meet a11y requirements. I also added an underline on hover which was not requested, but makes it a bit more obvious it's a clickable link. No problem to remove it again! 🙂

The last span element inside a li element will have no text decoration or any other hover effect, see line 147 in the same file.

src/styles/theme/oc-text.scss Outdated Show resolved Hide resolved
@pascalwengerter pascalwengerter force-pushed the a11y-fix-sufficient-text-contrast branch from 4749f9d to 746c985 Compare January 25, 2022 16:55
Steve Frenzel added 2 commits January 25, 2022 18:00
@pascalwengerter
feat(OcBreadcrumb): darker contrast ratio and underline on hover
d26cb93 
feat(color): higher contrast ratio on hover
@pascalwengerter
feat(oc-text): higher contrast ratio for links in dropdown menu
f9dcde1 
style(oc-text): switched to same color group

Remove underline from breadcrumb hover adjustments
@pascalwengerter pascalwengerter force-pushed the a11y-fix-sufficient-text-contrast branch from 746c985 to f9dcde1 Compare January 25, 2022 17:01
@pascalwengerter pascalwengerter merged commit 4260b40 into a11y-updates Jan 25, 2022
@delete-merged-branch delete-merged-branch bot deleted the a11y-fix-sufficient-text-contrast branch January 25, 2022 17:02
@pascalwengerter
Copy link
Contributor

Pretty sure the linked issue needs further adjustments around the "New" button, but merged this one for now

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pascalwengerter pascalwengerter pascalwengerter requested changes

@kulmann kulmann Awaiting requested review from kulmann

Assignees

@stevefrenzel stevefrenzel

Labels
3 - To Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@stevefrenzel @pascalwengerter @kulmann