feat: Identifier component MVP #1044
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…for IdentifierGov
…r; rendering text confirming import in Storybook
…ponent to display a description and a link to USA.gov
…or IdentifierLinks accepting custom styles
…lly render the correct language
…use a string to access properties of the content map object
…ers English) when passed no props
…code to IdentifierMasthead to pass the test
…ntifier parent component
…ybook when passed as props to IdentifierGov
…apshots taken for test purposes
brandonlenz
reviewed
Mar 30, 2021
Comment on lines
129
to
147
| <div className="usa-identifier__container"> | ||
| <div className="usa-identifier__logos"> | ||
| <IdentifierLogo | ||
| agencyUrl="#" | ||
| className="usa-identifier__logo-custom-class-name"> | ||
| {testIdentifierLogo} | ||
| </IdentifierLogo> | ||
| </div> | ||
| <div | ||
| data-testid="identifierMasthead-agency-description" | ||
| className="usa-identifier__identity" | ||
| aria-label="Agency description"> | ||
| <p className="usa-identifier__identity-domain">domain.edu.mil.gov</p> | ||
| <p className="usa-identifier__identity-disclaimer"> | ||
| {`An official website of the `} | ||
| <a href="testlink">Test Agency Name</a> | ||
| </p> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
I like this flexible approach as it does provide value to the consumer, and it removes a complex burden of hardcoding grammar for multiple languages.
There is an enhancement I wanted to surface for reviewers to discuss. I don't think that we necessarily need to do this for this PR, but my inclination would be to create wrapper components for most of this JSX, just to remove the burden on the implementer from needing to interact with the uswds classes. That's one of the main/nice things about our library is that we encapsulate all of that into our components, generally.
That would involve creating several subcomponents like IdentifierContainer, IdentifierLogos, IdentifierAgencyDescription.
If that is something we agree would be a good idea, I would be happy with creating a Github issue for the enhancement, and merging this as is for now. Then it wouldn't be a breaking change in the future, just a slightly more polished out-of-the-box set of sub-components.
Thoughts?
There was a problem hiding this comment.
And @SirenaBorracha just to be clear, on this point I'm definitely not expecting you to make any changes for this at the moment. Curious what other reviewers think as well.
There was a problem hiding this comment.
@brandonlenz yup I definitely agree!
…e, update IdentifierLogo prop agencyUrl to simply href and update everywhere, all hrefs now point to # and eslint check is disabled to reflect that in Identifier.stories
suzubara
previously approved these changes
Mar 31, 2021
| export const Identifier = ({ | ||
| className, | ||
| children, | ||
| }: IdentifierProps & JSX.IntrinsicElements['div']): React.ReactElement => { |
There was a problem hiding this comment.
non-blocking suggestion: We may want to allow users to pass additional props to the <Identifier> component that get passed along to the div, like how sectionProps are used in the IdentifierGov component below
| 'usa-identifier__logo', | ||
| className, | ||
| anchorClassName | ||
| ) |
There was a problem hiding this comment.
Since the anchor (<a>) is the intrinsic element for this component, can we structure this like we do with the other components where additional props (and className) are passed along directly to the <a> element? I don't think anchorClassName is needed here because className serves the same purpose, and anchorProps can be the used to define the "rest" of the props (ie, ...anchorProps) instead of a single named prop. For example, to the user it's the difference between:
<IdentifierLogo anchorProps={{ ariaLabel: "My logo" }}> ...
and:
<IdentifierLogo aria-label="My logo"> ...
…e rest of the components including JSX.IntrinsicElements in function signature instead of passed in props, update Identifier to take in and spread a divProps property on the parent div to handle anything the user might want to pass in to be applied to the entire component
haworku
previously approved these changes
Apr 1, 2021
package.json
Outdated
| @@ -197,5 +197,6 @@ | |||
| "hidden": true | |||
| } | |||
| ] | |||
| } | |||
| }, | |||
| "dependencies": {} | |||
There was a problem hiding this comment.
oh no, I think this can be deleted
There was a problem hiding this comment.
I see this removed in 8e813b8 now. Thanks @SirenaBorracha!
…ks/react-uswds into ak-new-component-Identifier-737
brandonlenz
approved these changes
Apr 5, 2021
Merged
brandonlenz
added a commit
that referenced
this pull request
Apr 12, 2021
## [1.15.0](1.14.0...1.15.0) (2021-04-12) ### Features * Identifier component ([#1044](#1044)) ([e79bc87](e79bc87)) * StepIndicator component ([#1047](#1047)) ([d61988e](d61988e)) * TimePicker component ([#1082](#1082)) ([c7bfdee](c7bfdee)) * Update to USWDS 2.9.0 ([#1048](#1048)) ([3859eea](3859eea)) ### Documentation & Examples * Add guidance for PR titles and testing in an application ([#1028](#1028)) ([be3bed4](be3bed4))
brandonlenz
added a commit
that referenced
this pull request
May 12, 2021
* Add files for New Component: Indentifier, wrote and passing one test for IdentifierGov * Adding Storybook example for default Identifier rendering IdentifierGov * Adding Storybook example for default Identifier rendering IdentifierGov * Moving comments to the bottom of each file * Adding IdentifierMasthead and IdentifierLinks, importing to Identifier; rendering text confirming import in Storybook * Adding test for ability to pass in section attributes via props * d * Add template for each Storybook example; hard coded IdentifierGov component to display a description and a link to USA.gov * Adding props to IdentifierLinks to pass links as props * Adding test for passing attributes as props * Adding placeholder links array to Identifier component, adding test for IdentifierLinks accepting custom styles * Adding test checking for passed in language prop * Adding content map for IdentifierGov component copy * Rendering imported copy from content map in IdentifierGov component * Adding storybook examples matching USWDS * Forgot to push my changes yesterday * Passing test for correct copy based on passed in language * Noting established pattern in existing component * Updating the content mapobject naming to make it easier to conditionally render the correct language * Moving interface to content file, using Record to create safe way to use a string to access properties of the content map object * Adding another test checking for IdentifierGov default behavior (renders English) when passed no props * Adding test checking for passing in attributes via props, adding the code to IdentifierMasthead to pass the test * Adding bare bones structure based on USWDS code to IdentifierMasthead * Some cleanup in the content file, adding classnames method to the Identifier parent component * Adding children components to storybook, rendering in spanish in storybook when passed as props to IdentifierGov * Commiting yarn build updates to yarn.lock * Removing duplicate IdentifierGov test * Rendering complete IdentifierGov component in storybook, commiting snapshots taken for test purposes * Adding test checking for ability to pass in a className, commiting updated snapshots * Moving IdentifierGov copy map back to parent file, updating snapshots * Add test checking for ability to pass in custom className to IdentifierMasthead component * Import subcomponents to Identifier.test in order to pass render check, update IdentifierMasthead test for aria-label to actually test for custom label, update IdentifierGov test to be more elegantly worded * Add test to IdentifierMasthead to check for ability to pass in Spanishlanguage prop * Update Identifier test wording to be more specific * Replace hardcoded text with reference to copy object, include update to Identifier.test test props that I forgot to save in the previous commit * Add IdentifierMasthead props to Identifier.stories and Identifier.test, fix spacing between identityDisclaimer and the link to the parentAgency * Add optional boolean for logo and second logo to IdentifierMasthead, add tests for no logo, passed in logo, and multiple logos, also add new props to Identifier.stories rendering appropriate amount of logos and in appropriate language * Clean up comments, add note re: necessity of supporting more than two logos * Add appropriate props to multiple-logos + Spanish Storybook example * Add IdentifierLinkItem subfolder and files, importing to IdentifierLinks and rendering a div * Nest IdentifierLinks files appropriately; add IdentifierLink, IdentifierLinkItem subcomponents * Add IdentifierLink and tests following conventions established in BreadcrumbLink component * Add tests for IdentifierLinkItem, remove wayward aria-label and associated test * Remove superfluous data-testids * Add Logo folder and files with mostly pseudocode * Update Identifier logo subcomponent name to IdentifierLogo to differentiate between it and the Logo in Footer * Update Identifier.stories to pass links into IdentifierLinks properly, import IdentifierLogo into IdentifierMasthead * Remove comments, refactored code, update IdentifierMasthead in .stories * Add tests for IdentifierLogo checking for ability to pass in props, add map accounting for multiple parent agencies * Add new IdentifierMasthead test data to pass check in Identifier.test, add USWDS’ taxpayer disclaimer sample text to Identifier.stories, add conditional taxpayer disclaimer prop and conditional rendering for parent agencies to IdentifierMasthead * Add usa-link class to IdentifierLink and amend tests to check for usa-class * Update IdentifierLinks test to use subcomponents, remove DRY testvars in IdentifierLink in favor of DAMP * Add query to IdentifierLinks test, update IdentifierMasthead tests to reflect MVP check for rendering one agency and one logo * Conditionally render taxpayer disclaimer prop in IdentifierMasthead, update Identifier.stories to render subcomponents with USWDS example text * Add check for IdentifierLinks - render its subcomponents and render a custom link * Add test to Identifier to check for ability to render without errors in English and Spanish, and render without a logo * Add testParentAgencyNoLogos for no logo storybook examples * Move taxpayer disclaimer out of the parent agency link * Respond to PR feedback: move div in Masthead, remove commented out import in index.ts, remove disable in .stories, update .stories export to match new convention * Export all new Identifier components within index.ts * Remove IdentifierMasthead content map, transfer those props to the ParentAgency interface * Update Identifier.test and IdentifierMasthead.test with new test data * Fix Identifier test query, remove deprecated test data * Place logos div inside of conditional logo rendering * Refactor IdentifierLogo out of IdentifierMasthead file, passing it in and rendering as children in .stories file * Refactor agency interface out of IdentifierMasthead, refactor test agency objects out of Identifier.stories, passing directly into components in stories * Move container div from IdentifierMasthead into Identifier.stories * Remove ariaLabel prop from IdentifierMasthead * Remove superfluous test data from IdentifierMasthead tests * Remove test data from IdentifierMasthead tests again * Refactor content out of IdentifierGov, add example content to .stories * Refactor img out of IdentifierLogo, may be able to get rid of this component altogether, TBD * Render storybook examples for multiple parents and logos, english and spanish; update naming convention to remove the word identifier from storybook examples except for Default and Spanish * Refactor hardcoded aria-label out of IdentifierLinks, passing it in via navProps in each storybook example with English and Spanish * Add storybook example for more than two parent agencies, add storybook example for multiple parent agencies plus tax disclaimer copy * Update Identifier tests to reflect refactored subcomponents and structure, add tests checking for cases with no logos, cases with more than two logos * Remove superfluous import * Update IdentifierMasthead tests with new structure and test data, check for rendering without errors, rendering in Spanish, rendering without a logo, rendering more than two logos * Update Identifier.stories test data props * Update test variable declarations, the test images didn't need to be arrays * Update IdentifierLogo test file * Remove comment from IdentifierLinks test file, pass a string instead of the test variable so expected doesn't point to the same place as actual * Update IdentifierGov test file * Fixes from PR feedback; remove custom className from storybook example, update IdentifierLogo prop agencyUrl to simply href and update everywhere, all hrefs now point to # and eslint check is disabled to reflect that in Identifier.stories * Add fixes from PR feedback from Suz; structure IdentifierLogo like the rest of the components including JSX.IntrinsicElements in function signature instead of passed in props, update Identifier to take in and spread a divProps property on the parent div to handle anything the user might want to pass in to be applied to the entire component * Remove unnecessary dependencies array from package.json Co-authored-by: Brandon Lenz <brandonalenz@gmail.com>
brandonlenz
added a commit
that referenced
this pull request
May 12, 2021
## [1.15.0](1.14.0...1.15.0) (2021-04-12) ### Features * Identifier component ([#1044](#1044)) ([e79bc87](e79bc87)) * StepIndicator component ([#1047](#1047)) ([d61988e](d61988e)) * TimePicker component ([#1082](#1082)) ([c7bfdee](c7bfdee)) * Update to USWDS 2.9.0 ([#1048](#1048)) ([3859eea](3859eea)) ### Documentation & Examples * Add guidance for PR titles and testing in an application ([#1028](#1028)) ([be3bed4](be3bed4))
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds the Identifier component from USWDS.
I'm not sure about my Storybook examples, some may be unnecessary (others might be missing), and I believe the test suite could be more complete.
We decided against refactoring
IdentifierLinks/IdentifierLinkItem/IdentifierLinkas they follow currently practice using customLink- if a simpler way is obvious, please call it out.Related Issues or PRs
#737
How To Test
yarn storybookyarn testOr, click "show environments" to view this component in Storybook
Screenshots
Default, English and Spanish
No logos, English and Spanish
Multiple parent agencies and logos, English and Spanish
With tax disclaimer, English and Spanish
More than two parents and logos // Tax disclaimer and multiple parents and logos