localgovdrupal / localgov_base

The base theme for LocalGov Drupal websites.
8 stars 14 forks source link

feat: replaces standalone prev-next code with included version #588

Closed ctorgalson closed 2 weeks ago

ctorgalson commented 3 weeks ago

What does this change?

The template views-view-list--localgov-step-by-step-navigation--prev-next.html.twig now uses the _components/prev-next.twig template, bringing it in line with guides and the localgov-blogs block template.

This has several side-effects, all of them small, one of them (I think) positive:

How to test

Compare e.g. /adult-health-and-social-care/step-by-step/request-support-adult-step-step/apply-financial-support on an instance running this branch with the current demo site

How can we measure success?

This makes the markup and CSS more consistent with other identical UI components.

Have we considered potential risks?

There are two questions about the effects:

  1. is/was the aria-label on this specific component deliberate and/or important?
  2. is/was the difference in font-weight on this specific component deliberate and/or important?

Images

image

Left: existing implementation Right: this PR's implementation

Accessibility

My time on this provided by Annertech

msayoung commented 3 weeks ago

Hi @ctorgalson - thanks for this.

We do need the aria-labels to work, but I don't see the need for a specific template for it to do so, if we can get it working otherwise. Presumably we just need to set those variables in the template too?

{% set previous_title = prev_step_title %}

You probably know this, but the aria labels are there to give a better descriptive name and context for the link. https://www.w3.org/WAI/WCAG22/Understanding/link-purpose-in-context

ctorgalson commented 3 weeks ago

@msayoung Ah, sorry, I think I wasn't specific enough in my comments.

The links retain their aria-label attributes,* but in this change the <nav> element loses its aria-label.

As far as I can see, Step by Step is the only one of these Prev/Next <nav> elements (also used in Guides, and apparently somewhere in Blogs) that has an aria-label attribute.

Added later: the Publications <nav> (#564) has an aria-labelledby attribute that we have to consider as well.

But if we determine that the <nav> ought to retain the aria-label, then it'll be straightforward to add it into `templates/_components/prev-next.twig and provide suitable variables for all that template's various uses.


* Looking at the generated output, I see they include a harmless but superfluous : in some circumstances. I've addressed that here too (e9c9ea8).

markconroy commented 2 weeks ago

This makes perfect sense to me. We have another place or two as well where we can start transitioning things over to this new prev/next pattern so it's easier to maintain.


Thanks to Big Blue Door for sponsoring my time to work on this.