-
Notifications
You must be signed in to change notification settings - Fork 378
feat(Form): Add form related components #131
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
9c76c4a to
25126f2
Compare
|
Thanks for adding these, @sharvit! These examples look really nice!! To answer your second question, I think we should keep the Input Groups examples. We have these same examples on the patternfly test page for Forms Here are my comments based on reviewing your examples...
|
25126f2 to
c59510d
Compare
|
Thanks for your feedback @jgiardino !
👍
|
|
(testing the waffle board integration with this comment...) actually, adding 'closes #n' is not supported in the comment of a PR, so I added this to the description |
src/components/Form/Form.stories.js
Outdated
| const description = ( | ||
| <p> | ||
| This component is based on React Bootstrap Forms component. This component | ||
| represents a menu item in a dropdown. See{' '} |
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.
Bad copy/paste here I think.
c59510d to
2279f2c
Compare
|
@waldenraines - rebased and updated the description. |
waldenraines
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
Doh, I'm sorry, I just realized that the Terminology and Wording documentation shows the action capitalized as "Log in" which is probably confusing. I failed to mention that the button label should be headline style capitalization with a capital "I" in "Log In". The capitalization recommendations are a little further down on this page.
My concern with that approach is that we might not be able to consume many react-bootstrap components at all. :-) I don't think the patternfly-react components should necessarily prevent developers from accessing other variations that are available in react-bootstrap but are not supported in the patternfly design docs. I think we want to just not present these variations in the storybook so that the patternfly-react storybook always only shows patternfly-approved patterns. So in this case, I would suggest that we remove the KNOB for "Show Feedback Icon" and not have this property enabled in the storybook examples.
Thanks for sharing that example. I have totally overlooked it. :-) This one looks great to me! |
|
@priley86 - rebased Thanks for the feedback @jgiardino
That's fine, updated 👍
Agree, done. What do you think about exporting the |
2279f2c to
6384b66
Compare
|
@sharvit personally, I don't see the value of adding those iterators in the Vertical Form, Horizontal Form, and Inline Form examples just for the sake of making it look like a "real" form with all those fields. I would guess most devs will just be searching for the inner examples on how to use those elements/jsx. So maybe we can just simplify to one example of each variation w/o iterating them? Just personal opinion here... |
|
@priley86 I agree with you here, will wait to see how is the experience of using those components. |
|
No objections from me, @priley86. I just happened to click the link to the react-bootstrap documentation for forms, and the url must have recently changed from https://react-bootstrap.github.io/components.html#forms to https://react-bootstrap.github.io/components/forms/ Other than that, this LGTM! |
jeff-phillips-18
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
|
@jgiardino - Nice catch, seems they updated the whole UI. I have rebased, updated the URL and redeployed the storybook I guess it also means we need to update the urls everywhere? |
|
@sharvit @jgiardino this should be easy enough. We can write a helper and update all links in a subsequent PR. |
|
opened #154 to address the documentation links |

What:
Add form related components
closes #118, closes #117, closes #116
Link to Storybook
Discussion:
Fieldlevel components (e.g.Stories/HorizontalFormField.js), do you think those components should get exported to consumers?Additional issues: