-
Notifications
You must be signed in to change notification settings - Fork 18
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: aria-label class not properly hidden (fixes #606) #611
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀 looks good
@@ -3,33 +3,28 @@ | |||
.component { position: relative; } | |||
.block { position: relative; } | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the bootstrap example, is it worth adding a comment to instruct developers on its usage.
// Use to hide content visually while keeping it accessible to assistive technologies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
less/core/accessibility.less
Outdated
display: none !important; | ||
.aria-label-focusable, .visually-hidden-focusable { | ||
&:not(:focus):not(:focus-within) { | ||
.visually-hidden |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.visually-hidden
includes pointer-events: none;
which we probably want to exclude if we intend to use this for clickable buttons e.g. "skip navigation". In which case we could utilise a mixin here. Please see suggestion below.
.visually-hidden() {
display: block;
position: absolute !important;
width: 1px !important;
height: 1px !important;
padding: 0 !important;
margin: -1px !important;
overflow: hidden !important;
clip: rect(0, 0, 0, 0) !important;
white-space: nowrap !important;
border: 0 !important;
}
.aria-label {
.visually-hidden();
pointer-events: none;
}
.aria-label-focusable, .visually-hidden-focusable {
&:not(:focus):not(:focus-within) {
.visually-hidden;
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
clip: rect(0, 0, 0, 0) !important; | ||
white-space: nowrap !important; | ||
border: 0 !important; | ||
pointer-events: none; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the bootstrap example, is it worth adding a comment to instruct developers on its usage.
// Use to only display content when it's focused
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works as expected thanks @hanshillen but I've added some comments for your input please.
I've addressed the requested changes. |
fixes #606
Fix
.aria-label
per discussion in aria-label class not properly hidden #606