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

Add anchor navigation feature to accordions #1937

Merged
merged 1 commit into from
Feb 18, 2021

Conversation

owenatgov
Copy link
Contributor

@owenatgov owenatgov commented Feb 17, 2021

What

Applies an optional anchor navigation feature to accordions. This allows anchor links within accordions to:

  • link to another accordion heading on the same page and automatically open that accordion
  • link to an id anchor within an accordion on the same page and automatically open the containing accordion
  • link to either of the above on different pages with the same setup (accordions with this feature flag) and open the associated accordion as above.

Additionally fixes a couple of typos in the accordions js and removes a line specifying data_attributes as nil. The latter is unnecessary and generates an extra null attribute on every item on the docs.

Why

On manuals, there was previously a function to link between individual sections in manuals for ease of content. This was done by linking via an id anchor and the old bespoke manuals accordion js would work this out and open the accordion. This feature was overlooked in the recent update to manual accordions, leaving a lot of broken links and potentially confusing user journeys across manuals. There's a question to be answered about how accessible this functionality is, however for the moment this PR repairs the bug without having to undo the accordion changes and re-introducing a WCAG 2.3.4 failure.

No visual changes.

@bevanloon bevanloon temporarily deployed to govuk-publis-accordion--eva81p February 17, 2021 12:33 Inactive
@owenatgov owenatgov changed the title Add option to add ids to headings on accordion Add option to include id on accordion heading Feb 17, 2021
@owenatgov owenatgov marked this pull request as ready for review February 17, 2021 12:34
@bevanloon bevanloon temporarily deployed to govuk-publis-accordion--eva81p February 17, 2021 12:34 Inactive
@owenatgov owenatgov force-pushed the accordion-heading-id-functionality branch from 7e954da to 1574418 Compare February 17, 2021 14:00
@bevanloon bevanloon temporarily deployed to govuk-publis-accordion--eva81p February 17, 2021 14:00 Inactive

This attribute is paired with functionality to automatically open an accordion section on page load where the URL hash is the same as the id of that accordion. [Live example](https://www.gov.uk/guidance/how-to-publish-on-gov-uk/creating-and-updating-pages#associations)

Unlike with the accordion-wide custom id attribute, this id isn't stored in `localStorage` so does not need to be unique across your domain, but should still be unique to the page.
Copy link
Contributor

@chris-gds chris-gds Feb 17, 2021

Choose a reason for hiding this comment

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

This is great to mention this unique aspect. I'm wondering if this should be thought about when it comes to the API work.

I'm thinking about the ease of duplicate IDs happening..

Copy link
Contributor

Choose a reason for hiding this comment

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

Rails has a thing for generating random values that are unique called SecureRandom. I don't think it'd be useful here (looks like if no id is passed no id is created) but thought it worth mentioning.

Copy link
Contributor

@chris-gds chris-gds Feb 17, 2021

Choose a reason for hiding this comment

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

Ahh thanks @andysellick - good to know, to give some context in a publishing app an ID might be passed to link elsewhere -

## Add associations (tagging) {:#associations}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remember that there's a lot we still don't know. Manuals publisher might have validation for duplicate ids. This wouldn't surprise me to be honest, it's a feature in Whitehall of all places! I don't think policing data here is a valuable use of time.

Copy link
Contributor

@andysellick andysellick left a comment

Choose a reason for hiding this comment

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

Drive by review, couple of comments/questions. Also I'd suggest squashing your two commits together as they're part of the same change.


This attribute is paired with functionality to automatically open an accordion section on page load where the URL hash is the same as the id of that accordion. [Live example](https://www.gov.uk/guidance/how-to-publish-on-gov-uk/creating-and-updating-pages#associations)

Unlike with the accordion-wide custom id attribute, this id isn't stored in `localStorage` so does not need to be unique across your domain, but should still be unique to the page.
Copy link
Contributor

Choose a reason for hiding this comment

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

Rails has a thing for generating random values that are unique called SecureRandom. I don't think it'd be useful here (looks like if no id is passed no id is created) but thought it worth mentioning.

spec/components/accordion_spec.rb Outdated Show resolved Hide resolved
spec/components/accordion_spec.rb Show resolved Hide resolved
@owenatgov owenatgov force-pushed the accordion-heading-id-functionality branch from 1574418 to 4663a70 Compare February 17, 2021 19:31
@bevanloon bevanloon temporarily deployed to govuk-publis-accordion--eva81p February 17, 2021 19:31 Inactive
@owenatgov owenatgov changed the title Add option to include id on accordion heading Add anchor navigation feature to accordions Feb 18, 2021
@owenatgov owenatgov force-pushed the accordion-heading-id-functionality branch from 4663a70 to cb91159 Compare February 18, 2021 09:38
@bevanloon bevanloon temporarily deployed to govuk-publis-accordion--eva81p February 18, 2021 09:38 Inactive
@owenatgov
Copy link
Contributor Author

@andysellick Done #1941

@owenatgov owenatgov force-pushed the accordion-heading-id-functionality branch from cb91159 to a611517 Compare February 18, 2021 11:37
@bevanloon bevanloon temporarily deployed to govuk-publis-accordion--eva81p February 18, 2021 11:38 Inactive
@alex-ju alex-ju mentioned this pull request Feb 18, 2021
@@ -5,22 +5,22 @@
id ||= "default-id-#{SecureRandom.hex(4)}"
items ||= []
condensed ||= false
anchor_navigation ||= false
Copy link
Contributor

Choose a reason for hiding this comment

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

The flag is a nice halfway.

@@ -274,6 +274,35 @@ examples:
text: "How people read"
content:
html: "<p class='govuk-body'>This is the content for How people read.</p>"
with_the_anchor_link_navigation:
description: |
Some apps require custom ids per accordion section heading for linking between those specific sections, sometimes across multiple pages. An example of this is on manuals pages where multiple manuals will each include large sets of accordions and will reference between specific sections for ease of access to that content. [Live example](https://www.gov.uk/guidance/how-to-publish-on-gov-uk/creating-and-updating-pages#associations).
Copy link
Contributor

Choose a reason for hiding this comment

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

Further to our chat - I think accordions on manuals-frontend needs more consideration in general. We need to work out how we can do this feature, work out if the API changes can be done.

However for this issue - it's a good fix knowing what we know.

@owenatgov owenatgov force-pushed the accordion-heading-id-functionality branch from a611517 to 368f4a0 Compare February 18, 2021 16:46
@bevanloon bevanloon temporarily deployed to govuk-publis-accordion--eva81p February 18, 2021 16:46 Inactive
@owenatgov owenatgov merged commit 3bd03cc into master Feb 18, 2021
@owenatgov owenatgov deleted the accordion-heading-id-functionality branch February 18, 2021 16:49
DilwoarH pushed a commit that referenced this pull request Feb 18, 2021
Includes:
* Fix cookie banner preview in the component guide ([PR #1935](#1935))
* Implements scroll tracking for the covid pages ([PR #1942](#1942))
* Add anchor navigation feature to accordions ([PR #1937](#1937))
This was referenced Mar 17, 2021
chris-gds pushed a commit that referenced this pull request Jan 24, 2022
- Maintain existing GemAccoridon features of manuals-frontend that allow a user to navigation through this section. [1]
- Layering on features on-top of Design System Accoridon

[1] #1937
chris-gds pushed a commit that referenced this pull request Jan 24, 2022
- Maintain existing GemAccoridon features of manuals-frontend that allow a user to navigation through this section. [1]
- Layering on features on-top of Design System Accoridon

[1] #1937
chris-gds pushed a commit that referenced this pull request Jan 24, 2022
Update to follow conventions of GOVUK Frontend 4.0
Remove custom CSS for condensed layout, this is not in use on GOVK and goes against Design system Guidance. Additionally the accessibility impact has not been reviewed in depth.

- Maintain existing GemAccoridon features of manuals-frontend that allow a user to navigation through this section. [1]
- Layering on features on-top of Design System Accoridon
- Remove condensed layout, CSS and testing

[1] #1937
@chris-gds chris-gds mentioned this pull request Jan 24, 2022
2 tasks
chris-gds pushed a commit that referenced this pull request Feb 3, 2022
- Update GOVUK Frontend 4.0 version of Accordion

- Layering on GOVUK features on-top of Design System Accordion

- Add nodeListForEach, still required

- Restore polyfill, still required

- Remove custom CSS for condensed layout, this is not in use on GOVK and goes against Design system Guidance. Additionally the accessibility impact has not been reviewed in depth.

- Maintain existing GemAccoridon features of manuals-frontend that allow a user to navigation through this section. [1]

[1]#1937
chris-gds pushed a commit that referenced this pull request Feb 3, 2022
- Update GOVUK Frontend 4.0 version of Accordion

- Layering on GOVUK features on-top of Design System Accordion

- Add nodeListForEach, still required

- Restore polyfill, still required

- Remove custom CSS for condensed layout, this is not in use on GOVK and goes against Design system Guidance. Additionally the accessibility impact has not been reviewed in depth.

- Maintain existing GemAccoridon features of manuals-frontend that allow a user to navigation through this section. [1]

[1]#1937
chris-gds pushed a commit that referenced this pull request Feb 3, 2022
- Update GOVUK Frontend 4.0 version of Accordion

- Maintain nodeListForEach, still required

- Remove custom CSS for condensed layout, this is not in use on GOVK and goes against Design system Guidance. Additionally the accessibility impact has not been reviewed in depth.

- Layering on GOVUK features on-top of Design System Accordion

- Maintain existing GemAccoridon features of manuals-frontend that allow a user to navigation through this section. [1]

[1]#1937
chris-gds pushed a commit that referenced this pull request Feb 4, 2022
- Update GOVUK Frontend 4.0 version of Accordion

- Maintain nodeListForEach, still required

- Remove custom CSS for condensed layout, this is not in use on GOVK and goes against Design system Guidance. Additionally the accessibility impact has not been reviewed in depth.

- Layering on GOVUK features on-top of Design System Accordion

- Maintain existing GemAccoridon features of manuals-frontend that allow a user to navigation through this section. [1]

[1]#1937
chris-gds pushed a commit that referenced this pull request Feb 7, 2022
- Update GOVUK Frontend 4.0 version of Accordion

- Maintain nodeListForEach, still required

- Remove custom CSS for condensed layout, this is not in use on GOVK and goes against Design system Guidance. Additionally the accessibility impact has not been reviewed in depth.

- Layering on GOVUK features on-top of Design System Accordion

- Maintain existing GemAccoridon features of manuals-frontend that allow a user to navigation through this section. [1]

[1]#1937
chris-gds pushed a commit that referenced this pull request Feb 7, 2022
- Update GOVUK Frontend 4.0 version of Accordion

- Maintain nodeListForEach, still required

- Remove custom CSS for condensed layout, this is not in use on GOVK and goes against Design system Guidance. Additionally the accessibility impact has not been reviewed in depth.

- Layering on GOVUK features on-top of Design System Accordion

- Maintain existing GemAccoridon features of manuals-frontend that allow a user to navigation through this section. [1]

- Temporarily update to Tabs to resolve testing errors. Part of 4.0 update.

[1]#1937
chris-gds pushed a commit that referenced this pull request Feb 14, 2022
- Update GOVUK Frontend 4.0 version of Accordion

- Maintain nodeListForEach, still required

- Remove custom CSS for condensed layout, this is not in use on GOVK and goes against Design system Guidance. Additionally the accessibility impact has not been reviewed in depth.

- Layering on GOVUK features on-top of Design System Accordion

- Maintain existing GemAccoridon features of manuals-frontend that allow a user to navigation through this section. [1]

- Temporarily update to Tabs to resolve testing errors. Part of 4.0 update.

[1]#1937
chris-gds pushed a commit that referenced this pull request Feb 14, 2022
- Update GOVUK Frontend 4.0 version of Accordion

- Maintain nodeListForEach, still required

- Remove custom CSS for condensed layout, this is not in use on GOVK and goes against Design system Guidance. Additionally the accessibility impact has not been reviewed in depth.

- Layering on GOVUK features on-top of Design System Accordion

- Maintain existing GemAccoridon features of manuals-frontend that allow a user to navigation through this section. [1]

- Temporarily update to Tabs to resolve testing errors. Part of 4.0 update.

[1]#1937
chris-gds pushed a commit that referenced this pull request Feb 15, 2022
- Update GOVUK Frontend 4.0 version of Accordion

- Maintain nodeListForEach, still required

- Remove custom CSS for condensed layout, this is not in use on GOVK and goes against Design system Guidance. Additionally the accessibility impact has not been reviewed in depth.

- Layering on GOVUK features on-top of Design System Accordion

- Maintain existing GemAccoridon features of manuals-frontend that allow a user to navigation through this section. [1]

- Temporarily update to Tabs to resolve testing errors. Part of 4.0 update.

[1]#1937
chris-gds pushed a commit that referenced this pull request Feb 16, 2022
- Update GOVUK Frontend 4.0 version of Accordion

- Maintain nodeListForEach, still required

- Remove custom CSS for condensed layout, this is not in use on GOVK and goes against Design system Guidance. Additionally the accessibility impact has not been reviewed in depth.

- Layering on GOVUK features on-top of Design System Accordion

- Maintain existing GemAccoridon features of manuals-frontend that allow a user to navigation through this section. [1]

- Temporarily update to Tabs to resolve testing errors. Part of 4.0 update.

[1]#1937
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