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

feat: InputGroup component #2383

Merged
merged 9 commits into from
May 9, 2023
Merged

feat: InputGroup component #2383

merged 9 commits into from
May 9, 2023

Conversation

jcbcapps
Copy link
Contributor

@jcbcapps jcbcapps commented May 3, 2023

Summary

Related Issues or PRs

Resolves #1692

How To Test

Check unit tests

I did not add a Storybook component for this, but rather updated the InputPrefix and InputSuffix components to use InputGroup instead of a div. Happy to come back and add a Storybook component, but wasn't sure how to approach that in this context. Let me know if I need to make that change!

Screenshots (optional)


import { InputGroup } from './InputGroup'

describe('InputGroup component', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a test for checking that the right className is applied?

Copy link
Contributor

@brandonlenz brandonlenz May 3, 2023

Choose a reason for hiding this comment

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

also one with the error state

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I was just thinking the tests could probably be more thorough. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

@jcbcapps sorry, I shoulda been more clear. Can you add a test that verifies that the component has the default class name applied? (usa-input-group)

It can go in the renders without errors test, imo, since it's jsut verifying that the component built with the right uswds class

@brandonlenz
Copy link
Contributor

Love it! The more we create components for these classes, the more reacty and intuitive our library becomes to use! I might be preemptively commenting things since I saw another commit come in for something I was gonna mention. So far so good! I'll wait on the CI pipeline before approving!

@jcbcapps jcbcapps requested a review from brandonlenz May 3, 2023 21:30
Copy link
Contributor

@brandonlenz brandonlenz left a comment

Choose a reason for hiding this comment

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

One final thing, added a comment to thread about tests


import { InputGroup } from './InputGroup'

describe('InputGroup component', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

@jcbcapps sorry, I shoulda been more clear. Can you add a test that verifies that the component has the default class name applied? (usa-input-group)

It can go in the renders without errors test, imo, since it's jsut verifying that the component built with the right uswds class

@jcbcapps jcbcapps requested a review from brandonlenz May 9, 2023 15:59
@jcbcapps jcbcapps merged commit 5761db6 into main May 9, 2023
@jcbcapps jcbcapps deleted the 1692-jc-input-group-component branch May 9, 2023 18:38
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.

New Component: InputGroup
2 participants