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
base: develop
Are you sure you want to change the base?
Conversation
- This will fix a bug in html-templates build
There was a problem hiding this 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
packages/usa-identifier/src/content/usa-identifier~multiple-logos-lang-es.json
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this 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, |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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!
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. |
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 |
There was a problem hiding this 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
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:
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
withnull
.Testing and review
html-templates
directory renders as expectednpm run build:html
and confirm the HTML in the affected identifier files have the correct information