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 - Identifier: Fix content prefix bug in html-templates #5857

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

amyleadem
Copy link
Contributor

@amyleadem amyleadem commented Apr 9, 2024

Summary

Fixed a bug that added the English word "An" to Spanish variants of the identifier component. This was accidentally added to our component preview templates because of a data error.

Breaking change

This is a markup change that shouldn't be breaking.

Related issue

Closes #5893

Related pull requests

Changelog PR

Preview link

Storybook

Preview in uswds-site

Problem statement

The Spanish version of the identifier component needs to be updated to reflect the following changes:

  1. Take out "An" from "An Un sitio web oficial de ..."

Solution

Removed the word "An" from Spanish versions of the component. This was accidentally added to the html-templates version because the data did not explicitly define content_prefix with null.

Testing and review

  • Confirm the HTML in the html-templates directory renders as expected
    • Check that the component preview and component code on the demo identifier page display with the updated information
    • In this branch, run npm run build:html and confirm the HTML in the affected identifier files have the correct information
  • Confirm that the Storybook build renders as expected

- This will fix a bug in html-templates build
@amyleadem amyleadem marked this pull request as ready for review April 10, 2024 17:46
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 is looking good to me! One question about using Un as the prefix instead of null but everything else is looking good.

Since the link works with a redirect in it's current state, I was leaning that this could be a patch fix, however, thinking about how widespread the typo could be we'll definitely want to flag it and that might necessitate a minor version.

Clicking through some showcase links, I was able to find a few identifiers whose identifiers didn't have the An and were displaying correctly, so maybe the issue isn't as widespread and we might worry.

Testing checklist

  • Confirm the HTML in the html-templates directory renders as expected
    • Check that the component preview and component code on the demo identifier page display with the updated information
    • In this branch, run npm run build:html and confirm the HTML in the affected identifier files have the correct information
  • Confirm that the Storybook build renders as expected
  • Confirm there are no references to "Visite USA.gov en Español" or "https://www.usa.gov/espanol/" in uswds

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!

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.

Minor comment, but not a blocker — just a suggestion.


Also, the breaking change section in PR description could use some clarity.

This is not a breaking change.
This is a markup change.

This means that there are two changes total and one is minor (template prefix) and the other breaking (markup) — correct?

Important
This might need to be bumped to 3.9.0 because this involves markup changes. We could also explore splitting this into two different PRs - one to fix the erroneous "An" in the component preview in 3.8.1 and one for the intentional text updates in 3.9.0.

Splitting it into two makes sense to me. Especially if we want to follow semver. Any preference @thisisdano?

@@ -5,6 +5,7 @@
"masthead": {
"aria_label": "Identificador de la agencia,",
"description": "Descripción de la agencia,",
"content_prefix": null,
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency, it seems like the Spanish prefix here would be Un.

Copy link
Contributor

Choose a reason for hiding this comment

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

I brought up the same thing in this thread but Amy clarified that this attribute is used to ignore An in an official because it sounds like "unofficial". We shouldn't need to ignore the prefix in the Spanish variant!

@amyleadem
Copy link
Contributor Author

Update: I am going to work on splitting this into two separate PRs. This one will remain open for 3.8.1 and I will open a separate one for 3.9.0. Will post the additional PR here when it is ready.

@amyleadem
Copy link
Contributor Author

Update: I have created a new issue for this bug (#5893) and have broken out the related link update in PR #5892.

@mahoneycm and @mejiaj This is ready for your re-review

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 splitting up the PR. Helps a lot with testing.

Confirming the output Spanish HTML is no longer getting the English prefix.

<!-- On `npm run build:html` →
- <span aria-hidden="true">An </span>Un sitio web oficial de
+ Un sitio web oficial de

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: Remove "An" from Spanish variants of the identifier
3 participants