-
Notifications
You must be signed in to change notification settings - Fork 81
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
fix: adding labels in combo box (default combo box, with default value) #2636
Conversation
@trussworks/react-uswds-design-owners I'll review this PR 👍 |
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.
Great job on this - LGTM!
The only thing I might consider either as part of this PR or a separate one is potentially removing the With Label
use case since the default combo box already includes a label. It might help clean up the documentation; however, it might impact teams already using that component.
@@ -72,7 +74,7 @@ export const withLabel = (): React.ReactElement => { | |||
|
|||
return ( | |||
<Form onSubmit={noop}> | |||
<Label htmlFor="fruit">Select a fruit</Label> | |||
<Label htmlFor="fruit">Select a Fruit</Label> |
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.
The labels should be sentence case (see USWDS example):
<Label htmlFor="fruit">Select a Fruit</Label> | |
<Label htmlFor="fruit">Select a fruit</Label> |
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.
(Same for the others added on this PR)
Hello @brandonlenz, Can you kindly review my PR |
Summary
According to the USWDS docs the combo box should always have a label. Some of the examples in the storybook do not have labels. So the commit adds labels in the component file
src/components/forms/ComboBox/ComboBox.stories.tsx
to avoid confusion that they are optional.Related Issues or PRs
Closes #2608
How To Test
Screenshots (optional)