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

♿ #1084 Check borders after accessibility changes #463

Merged
merged 9 commits into from
Feb 14, 2023

Conversation

jiromaykin
Copy link
Contributor

@jiromaykin jiromaykin commented Feb 7, 2023

Some borders need to be reverted to original design, because: The 'Toegankelijkheidsaudit' does not explicitly ask for borders around these areas + This Level AA requirement reads: "...Incidental: Text[...]that are part of an inactive user interface component, that are pure decoration,[...], have no contrast requirement..."
"pure decoration" in the definition of WCAG = "serving only an aesthetic purpose, providing no information, and having no functionality"

https://taiga.maykinmedia.nl/project/open-inwoner/task/1084

Answer to my question on Stack Overflow: https://stackoverflow.com/questions/75379770/do-borders-that-visually-group-related-content-have-a-contrast-requirement

“... WCAG does not require you to have a border around the grouped elements. […] there aren't any WCAG requirements for the contrast. 1.4.11 Non-text Contrast comes close but that requirement is for "Interface Components" and "Graphical Objects". Your tile might contain "Interface Components" (such as a link) but the tile itself is not an "Interface Component" so 1.4.11 does not apply. That doesn't mean you shouldn't try to have better contrast, but it's not required by WCAG.”

@codecov-commenter
Copy link

codecov-commenter commented Feb 7, 2023

Codecov Report

Merging #463 (a868248) into develop (9e8c1bd) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop     #463   +/-   ##
========================================
  Coverage    96.46%   96.46%           
========================================
  Files          505      505           
  Lines        18255    18255           
========================================
  Hits         17609    17609           
  Misses         646      646           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jiromaykin jiromaykin force-pushed the fix/1084-Check-borders-after-accessibility-changes branch from dda7ee7 to 798e448 Compare February 9, 2023 16:00
@jiromaykin
Copy link
Contributor Author

jiromaykin commented Feb 9, 2023

Some elements still do need a bigger accessibility contrast with a different color --color-mute (like "input" interface components and other form inputs) but others need to be reverted to the original design.

@jiromaykin jiromaykin requested a review from alextreme February 9, 2023 16:56
@jiromaykin jiromaykin marked this pull request as ready for review February 9, 2023 16:57
@jiromaykin jiromaykin marked this pull request as draft February 10, 2023 08:58
@jiromaykin jiromaykin force-pushed the fix/1084-Check-borders-after-accessibility-changes branch from 18acea9 to 9ca7f72 Compare February 10, 2023 11:57
@jiromaykin jiromaykin marked this pull request as ready for review February 10, 2023 12:53
@jiromaykin jiromaykin force-pushed the fix/1084-Check-borders-after-accessibility-changes branch from 22057d2 to 38a18fc Compare February 13, 2023 08:48
@jiromaykin jiromaykin requested review from vaszig and removed request for alextreme February 13, 2023 12:05
Copy link
Contributor

@vaszig vaszig left a comment

Choose a reason for hiding this comment

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

I assume that dropdowns have to remain the same for accessibility reasons, right? Maybe I am losing something in the task..

  • dropdowns

image

@@ -13,12 +13,12 @@
}

&__list-item {
border-bottom: var(--border-width) solid var(--color-mute);
border-bottom: var(--border-width) solid var(--color-gray);
Copy link
Contributor

Choose a reason for hiding this comment

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

--border-width is 2px, maybe we should adhere to the card and dropdown buttons size -> 1px?
If so, let's create a variable for border size and set it to 1px, just to be reusable.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right, I think this should become 1px as well; i will add to it.

@vaszig
Copy link
Contributor

vaszig commented Feb 13, 2023

Feedback in search page? It doesn't seem to have a border-radius set.

image

@jiromaykin
Copy link
Contributor Author

@vaszig Do you mean you think that there should be an extra border there?
I don't think so. When I look at the design in Invision I do not see a set border, so this should stay as is (also because that feedback text-area border would only be 'decorative' and not 'functional' for accessibility)

feedback

@jiromaykin
Copy link
Contributor Author

I assume that dropdowns have to remain the same for accessibility reasons, right? Maybe I am losing something in the task..

Yes, anything that is -actually- needed for accessibility ("Interface Components" and "Graphical Objects" such as form elements etc.) still needs that darker border, so i did not change it for the elements that still need it. I you feel like looking deeper into this, look at my Stackoverlflow question-with-answer linked in the description of this PR.

@vaszig
Copy link
Contributor

vaszig commented Feb 13, 2023

@vaszig Do you mean you think that there should be an extra border there? I don't think so. When I look at the design in Invision I do not see a set border, so this should stay as is (also because that feedback text-area border would only be 'decorative' and not 'functional' for accessibility)

image

No, I mean the border-radius here. Not adding extra borders. I see that the design has but we don't.

@jiromaykin
Copy link
Contributor Author

No, I mean the border-radius here. Not adding extra borders. I see that the design has but we don't.

Ah, I see, you mean the voting-labels; well spotted!
Yes, I will add a radius around those.

@alextreme
Copy link
Member

Thanks @vaszig for the detailed check! 👍

@jiromaykin jiromaykin requested a review from vaszig February 14, 2023 11:16
Copy link
Contributor

@vaszig vaszig left a comment

Choose a reason for hiding this comment

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

Only two small remarks ant I think it's ready to be merged. @alextreme can tell us his opinion as well.

@@ -12,16 +12,38 @@
color: var(--color-primary);
cursor: default;
}
&#negative {
Copy link
Contributor

Choose a reason for hiding this comment

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

We have nice buttons now!If you think the border on focus is a problem feel free to to fix it. I am not sure about it.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that is not the way it should be, eventually, but I want to collect that in a new ticket for all accessibility issues that are not quite on par yet.

@alextreme
Copy link
Member

Only two small remarks ant I think it's ready to be merged. @alextreme can tell us his opinion as well.

@vaszig My opinion is that we'll likely have to iterate on this, we won't be able to get this exactly according to the design in one go but I'm happy with these and other steps being taken.

@jiromaykin jiromaykin requested a review from vaszig February 14, 2023 15:26
@alextreme alextreme merged commit 33d9f36 into develop Feb 14, 2023
@alextreme alextreme deleted the fix/1084-Check-borders-after-accessibility-changes branch February 14, 2023 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants