Skip to content

Conversation

paulathevalley
Copy link
Contributor

@paulathevalley paulathevalley commented Nov 16, 2022

Description

Issue: Headings are not properly nested in the footer on OSG priorities pages. The usa-footer/src/index.js file assumes the footer headings will always be h4 elements.

This PR sets a data-tag attribute with the html tag on the new button element and reference it when toggling back to the original html tag.

Additional information

Before you hit Submit, make sure you’ve done whichever of these applies to you:

(👋 first PR! hello! thank you for your patience)

@paulathevalley paulathevalley marked this pull request as draft November 16, 2022 21:05
@paulathevalley paulathevalley marked this pull request as ready for review November 18, 2022 02:42
@paulathevalley
Copy link
Contributor Author

Apologies for opening the PR too early. Ready for review now.
cc @jkjustjoshing

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.

@paulathevalley thanks for the contribution!

Just a few questions:

  1. This allows users of big footer to use any heading element by simply updating header in .usa-footer__primary-link, right? For example, if I wanted to use h6 instead of h4 I'd have something like: <h6 class="usa-footer__primary-link">.
  2. Would you be able to add a test for this feature in footer.spec.js?

@paulathevalley
Copy link
Contributor Author

@mejiaj To answer your questions:

  1. Yes, exactly.
  2. Yes, can do!

Thank you for your time!

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.

Great, thank you so much @paulathevalley!

I've tested:

  • Footer can take any header, value gets saved as data attr on mobile and restored on desktop
  • Unit test passes

Copy link
Contributor

@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 improvement, thank you!

@thisisdano thisisdano merged commit 1589158 into uswds:develop Feb 28, 2023
@thisisdano thisisdano mentioned this pull request Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants