-
Notifications
You must be signed in to change notification settings - Fork 19
Feature/2.x/805 add default accessible label to templates using localgov prev next #806
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
Feature/2.x/805 add default accessible label to templates using localgov prev next #806
Conversation
- prev_next_nav_aria_label: convenience prop allowing aria-label to be set without needing to create an attribute object - prev_next_pre_content: slot allowing content to be inserted before the Prev and Next links inside the <nav> element - prev_next_post_content: slot allowing content to be inserted after the Prev and Next links inside the <nav> element
|
We have some test failures that we need to check out. Let's add to - "Have we considered potential risks?" I think the answer to both of these will be 'no'. |
markconroy
left a comment
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 looks really good, just a few small comments to look at and we're good to go.
templates/views/views-view-list--localgov-step-by-step-navigation--prev-next.html.twig
Outdated
Show resolved
Hide resolved
|
@ctorgalson still wants to tweak something on this one before it's ready. |
This prevents id collisions, but makes using the id for styling impossible. A fair trade-off given that it's a brand new feature (i.e. it can't be in use anywhere already).
|
@markconroy I think b8353f4 resolves the issues here if you want to give it another once-over. |
Taking the second first, since you and @markconroy suggested the same issue, I've added a random string to those generated ids. It will make them unique at the cost of using them as selectors. But since they're new, that's a non-issue. For the first point, I tried to think how the change could break a child-theme, but I couldn't come up with a way:
But if anyone thinks of a way it could go wrong, I'm happy to try to address it in the code (and acknowledge the creativity!) |
markconroy
left a comment
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.
LGTM
Re: #805
What does this change?
This makes several changes:
prevNextAttributesinprev-next.component.ymlprev_next_nav_aria_labelprop for convenience (i.e. since this can also be set usingprevNextAttributes)prev_next_pre_contentslot to add content before the Prev and Next linksprev_next_post_contentslot to add content after the Prev and Next linksprev_next_pre_contentslot to add a labelled, visually-hidden heading to Blog templateprev_next_pre_contentslot to add a labelled, visually-hidden heading to Guides templateprev_next_pre_contentslot to add a labelled, visually-hidden heading to Publication templateprev_next_pre_contentslot to add a labelled, visually-hidden heading to Step by Step templateThe new labels all use Drupal's stock pattern: an
aria-labelledbyattribute on the<nav>element referring to a visually-hidden child<h2>with a matchingidattribute.How to test
Test all of the following with demo content (not sure what module the Blog template requires):
How can we measure success?
<nav>elements wrapping the Prev-Next links will now have an accessible text label by defaultHave we considered potential risks?
For existing overrides of the template, a new
aria-labelledbyandh2will appear:aria-label), the<nav>'saria-labelledbyattribute plus its new label will override the theme's label; this is probably not serious but could be a problem where e.g. the new label string is not translated on a multilingual siteImages
n/a
Accessibility
Colour contrast passedThe change doesn't use only colour to convey meaning