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 - Remove id from site title #5319

Merged
merged 4 commits into from
Jun 8, 2023
Merged

Conversation

amyleadem
Copy link
Contributor

@amyleadem amyleadem commented Jun 5, 2023

Summary

  • Fixed a bug that prevented the header from finding the designated site_title content during the HTML build process. The site title appeared with the accurate value in Storybook, but showed the default Home value in the html files.
  • Removed the id attribute from the usa-logo element. This id required maintenance of multiple lines of JSON but did not seem to serve a clear purpose.
    • This might be too bold of an approach. If we prefer, we can keep the id attribute and just create a standard value for all uses.

Related PR

This was originally intended to be merged into #5311.

Preview link

Related PR

Changelog PR: uswds/uswds-site#2124

Testing and review

  • Confirm that the site title content in the header component is <Project title> for both Storybook and the following files in html-templates:
    • usa-header.html
    • usa-header~megamenu.html
    • usa-header--extended.html
    • usa-header--extended-megamenu.html
  • Consider if there is any need for the id in the site title and if it is advisable to remove.

@amyleadem amyleadem marked this pull request as ready for review June 6, 2023 14:17
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.

Looks good, I tested:

  • Output HTML from npm run build:html
  • StorybookJS components (headers, site title, and nav partial).

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.

lgtm!

  • Confirmed ID has been removed
  • Confirmed correct title is displaying in storybook as well as html templates after build:html
  • Confirmed USWDS is not referencing the ID in any existing code.

Base automatically changed from jm-extended-megamenu-bug-5305 to develop June 6, 2023 22:42
@amyleadem amyleadem added this to the uswds 3.5.0 milestone Jun 7, 2023
@amyleadem
Copy link
Contributor Author

amyleadem commented Jun 7, 2023

@thisisdano This PR was originally intended to be merged into #5311 so it does not have its own issue. #5311 has already been merged into develop, so I've tagged this as a separate 3.5.0 item.

@thisisdano thisisdano merged commit cf8c634 into develop Jun 8, 2023
@thisisdano thisisdano deleted the al-extended-megamenu-bug-5305 branch June 8, 2023 17:59
@thisisdano thisisdano mentioned this pull request Jun 9, 2023
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