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

Fix bug when getting target element from URL hash #2985

Merged
merged 1 commit into from
Sep 26, 2022

Conversation

MartinJJones
Copy link
Contributor

@MartinJJones MartinJJones commented Sep 23, 2022

What

Update the accordion component to only look for a target element within the module itself, not the entire document.

Why

This fixes a bug when anchor_navigation is set to true and an ID is passed in for an element that exists on the page, but is not contained within the accordion component.

For example: https://www.gov.uk/guidance/how-to-publish-on-gov-uk/creating-and-updating-pages#section-title

Further Info

Fixes: #2086
Trello: https://trello.com/c/q5P6u7id/1507-ps-17-accordion-anchor-link-navigation-breaks-with-invalid-anchor

Using querySelector when the ID contains a colon

There was also a need to escape the colon character when using querySelector, for example $this.module.querySelector('#fnref:1') would throw an error 1

Footnotes

  1. Bugzilla - querySelector doesn't work if the ID contains a colon (:)

@govuk-ci govuk-ci temporarily deployed to components-gem-pr-2985 September 23, 2022 15:01 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-2985 September 23, 2022 15:07 Inactive
@MartinJJones MartinJJones changed the title [WIP] Fix open for anchor bug and add tests Fix bug when getting target element from URL hash Sep 23, 2022
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-2985 September 23, 2022 15:45 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-2985 September 23, 2022 15:49 Inactive
@MartinJJones MartinJJones marked this pull request as ready for review September 23, 2022 15:54
Copy link
Contributor

@jon-kirwan jon-kirwan left a comment

Choose a reason for hiding this comment

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

Looks great thanks! I've added two minor suggestions but not critical changes by any means so have approved

@govuk-ci govuk-ci temporarily deployed to components-gem-pr-2985 September 26, 2022 12:08 Inactive
Copy link
Contributor

@jon-kirwan jon-kirwan left a comment

Choose a reason for hiding this comment

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

All good! 👍

When searching for the target element we now restrict this to the accordion component, instead of the searching the entire document.

The html fixture was also updated in the related test to add a closing div tag that was missing.
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-2985 September 26, 2022 14:22 Inactive
@MartinJJones
Copy link
Contributor Author

Amended note in the Changelog

@MartinJJones MartinJJones merged commit 09ab8f8 into main Sep 26, 2022
@MartinJJones MartinJJones deleted the fix-accordion-bug branch September 26, 2022 14:27
@JamesCGDS JamesCGDS mentioned this pull request Sep 27, 2022
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.

Accordion anchor link navigation breaks with invalid anchor
3 participants