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

USWDS - In-Page Navigation: Fix error when clicking link starting with number #5200

Merged
merged 1 commit into from May 8, 2023

Conversation

aduth
Copy link
Contributor

@aduth aduth commented Mar 27, 2023

Summary

Fix an error when clicking a link starting with a number. Certain links would not scroll as expected when clicked.

Related issue

Fixes #5199

Problem statement

See #5199

Solution

This implements the first of the proposed options outlined at #5199 (comment) .

This has a few advantages, in that it limits the scope of the changes and maintains backwards compatibility. The only downside that I can consider is that these IDs are still technically not valid for use as CSS selectors, in case someone would need to reference it as such. As demonstrated in the included tests, a workaround is to reference the ID as using an attribute selector (e.g. [id="1-installation"]).

Testing and review

  1. Run Mocha tests
  2. Perhaps this could be incorporated into demo content so it could be observed in the Storybook preview?

@amyleadem amyleadem requested a review from mejiaj March 27, 2023 15:09
@amyleadem amyleadem self-assigned this Mar 27, 2023
@amyleadem amyleadem self-requested a review March 27, 2023 15:09
@amyleadem amyleadem removed their assignment Mar 27, 2023
Copy link
Contributor

@mahoneycm mahoneycm left a comment

Choose a reason for hiding this comment

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

This works like a charm!

Tested:

  1. Added numbers to the start of headings in the storybook preview
  2. Clicked through in-page navigation to confirm it scrolled to the correct heading
  3. Confirmed no errors in the console
  4. Logged document.getElementById(el.hash.slice(1)) and confirmed the correct element was being selected.
  5. Ran gulp test for in-page navigation

Copy link
Contributor

@mejiaj mejiaj left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I think it would be useful to add it to the twig template for Storybook.

Tested:

  • Component works as expected in StorybookJS
  • No issues in unit test

Didn't mean to edit your comment Charlie, sorry!

Copy link
Contributor

@amyleadem amyleadem left a comment

Choose a reason for hiding this comment

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

Good improvement. Thank you!
Works as expected when headings have a numeric prefix in Safari, Edge, Chrome, Firefox.

I agree that it would be great to see an example of this reflected in the Storybook twig template.

Copy link
Member

@thisisdano thisisdano left a comment

Choose a reason for hiding this comment

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

Nice clean solution

@thisisdano thisisdano merged commit f0c1d61 into uswds:develop May 8, 2023
3 checks passed
@thisisdano thisisdano mentioned this pull request Jun 6, 2023
@aduth aduth deleted the aduth-5199-error-heading-number branch June 10, 2023 12:40
@aduth aduth restored the aduth-5199-error-heading-number branch June 10, 2023 12:40
@aduth aduth deleted the aduth-5199-error-heading-number branch June 10, 2023 12:40
@aduth aduth restored the aduth-5199-error-heading-number branch June 10, 2023 12:40
@aduth aduth deleted the aduth-5199-error-heading-number branch June 10, 2023 12:40
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.

USWDS - Bug: In-page navigation errors when heading starts with number
5 participants