Move the aria-expanded attribute to the correct element

[colinbm]

⚠️ Don't forget to update the gem version in the CHANGELOG before merging! When you're ready to release bump version file and generate a tag. ⚠️

What

• Move the aria-expanded attribute onto the toggle button

Why

From the DAC review:

There are sections of content with Show/ Hide functionality. Neither the expandable behaviour of the control nor its expanded or collapsed state are programmatically determinable as the aria-expanded attribute has not been utilised (please note, it appears this attribute has been incorrectly associated with the expandable region rather than the trigger).

[36degrees]

Whilst looking at this, I spotted an issue with the nav list that I've also fixed... currently we have too many ul elements. We should only have these at the open/close of the nav, and at the open/close of each new level of nav. As it is we have a ul element for each li.

I'm not sure this change is quite right – now everything in the nav sits at the same level.

On the current version in master, the nav looks like this:

On this branch, it looks like this:

[colinbm]

Odd - it's not doing that on mine - investigating.

[36degrees]

Can we consider splitting the list structure changes into a separate PR? As I understand it, they're unrelated to the aria-expanded change which looks good to go?

[colinbm]

@36degrees seperated that off

[36degrees]

Thanks @colinbm! Would you mind tidying up the PR description to remove the changes that have been split out?

Unfortunately, there's another place in the code where aria-expanded is being set which needs fixing too. When the code is first initialised, aria-expanded is still getting set to false on the collapsible__body:

tech-docs-gem/lib/assets/javascripts/_modules/collapsible-navigation.js

Lines 35 to 37 in 4ba92c8

	$(this).addClass('collapsible__body')
	.attr('id', uniqueId)
	.attr('aria-expanded', 'false')

This means that the collapsed state is not announced when you first navigate to a button, only once it's been toggled at least once.

We're also still including a non-sensical aria-expanded on the toggled region, except now it's permanently set to false.

Sorry that I didn't spot that as part of my previous review!

We should really have tests that cover all of this included in /spec/javascripts/collapsible-navigation-spec.js. However, I don't think it's fair to ask you add them as you're only fixing the existing implementation.

[colinbm]

@36degrees Ah, apologies - I had fixed that, but somehow it failed to go into the commit.

[36degrees]

👍🏻