-
Notifications
You must be signed in to change notification settings - Fork 375
feat(Input group): Simplified API #9176
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
Conversation
|
Preview: https://patternfly-react-pr-9176.surge.sh A11y report: https://patternfly-react-pr-9176-a11y.surge.sh |
| {...props} | ||
| > | ||
| {children} | ||
| {isBox ? <InputGroupText> {children} </InputGroupText> : children} |
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.
@mcoker I made the changes that we talked about. I still think we could improve this. I just don't think it is intuitive to me that applying isBox implies that we will be adding text as the input group item. I wonder if we should have a isText prop. If the consumer passes that in and they did not set "isBox" we can set it for them and render the <InputGroupText> internally as well.
| return ( | ||
| <InputGroup> | ||
| <InputGroupItem isFill>{textInput}</InputGroupItem> | ||
| <InputGroupItem isBox> |
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.
| className, | ||
| component = 'span', |
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.
Should we add props to InputGroupItem to pass down to InputGroupText? With the API update the className and component won't be able to be modified.
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.
cc @mcoker do you think that is necessary? What is the use case where we would not want to use span?
mcoker
left a comment
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!
gitdallas
left a comment
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, just a question
| section: components | ||
| cssPrefix: null | ||
| propComponents: ['InputGroup', 'InputGroupItem', 'InputGroupText'] | ||
| propComponents: ['InputGroup', 'InputGroupItem', InputGroupText] |
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.
What exactly does this do? I'm seeing in a few places
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.
it adds the prop docs at bottom of page.
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 meant the removal of quotes
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
* feat(Input group): Simplified API * updates from pr review * add isDisabled prop to InputGroupText * update snapshots * update from review --------- Co-authored-by: Titani <tlabaj@redaht.com>


What: Closes #9149