Skip to content

Conversation

@sharvit
Copy link
Contributor

@sharvit sharvit commented Dec 17, 2017

What:
Add form related components
closes #118, closes #117, closes #116

Link to Storybook

Discussion:

  1. I created some Field level components (e.g. Stories/HorizontalFormField.js), do you think those components should get exported to consumers?
  2. Do you think I should get rid of some InputGroups because they do not feel like patternfly?

Additional issues:

@sharvit sharvit changed the title Feature/forms feat(Form): Add form related components Dec 17, 2017
@jgiardino
Copy link
Contributor

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...
@serenamarie125 - Please also review these comments and make sure they line up with your expectations too.

  1. In the PatternFly styleguide for terminology and wording the recommendation is to use "Log In" as the button label for the Login form pattern that you show for the Inline Form.
  2. The PatternFly design for validation does not include icons in the validation states. When I modify the KNOBS so that a validation state is selected and Show Feedback Icon is enabled (i.e. in an example like Horizontal Form), icons display in the input. This is visually an issue in the field labeled "My meeting URL" which includes an example of an input group with a button inside the input field. In this case, the icon and button overlap (see example below). Can we remove the KNOB "Show Feedback Icon" from these examples?
    • I realize this option would still be available in the component, but we are hoping to avoid confusion with developers who want to implement patternfly patterns by not including any storybook examples that are not consistent with the patternfly examples.
    • image
  3. Some of the examples include a KNOB labeled "Show Loading" which will display a spinner on the form. I'm actually not familiar with any patternfly examples that show a spinner on a form, but there are a couple of things I noticed related to the spinner:
    • When using the inline spinner with a paragraph, the spinner-xs class should be used. Sizes per text elements are available on the patternfly spinner test page
    • The spacing between the spinner and text on the patternfly test page seems to created with just a space character. I would suggest following that, instead of applying the 10px padding to the text.
    • @serenamarie125 Are you aware of any examples that show a form layout with a spinner included, both for modal and non-modal forms? I'm not sure what the patternfly standard might be in this case.

@sharvit
Copy link
Contributor Author

sharvit commented Dec 19, 2017

Thanks for your feedback @jgiardino !

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

👍

  1. Updated to Log in

  2. Do you mean to remove the FormControlFeedback from all the stories?
    If so, I think I should remove this component from patternfly-react so it can be consumed from react-bootstrap or just add it as a normal markup. So we won't supply components which are not consistent with patternfly.

  3. I found the spinner example here (at the bottom of this page): http://www.patternfly.org/pattern-library/forms-and-controls/buttons-on-forms/

    • Updated the size.
    • Updated the spacing and removed the padding.

@jgiardino
Copy link
Contributor

jgiardino commented Dec 19, 2017

(testing the waffle board integration with this comment...)
closes #116
closes #117
closes #118


actually, adding 'closes #n' is not supported in the comment of a PR, so I added this to the description

@waldenraines
Copy link

@sharvit can you rebase this now that #130 is merged please?

const description = (
<p>
This component is based on React Bootstrap Forms component. This component
represents a menu item in a dropdown. See{' '}

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.

@sharvit
Copy link
Contributor Author

sharvit commented Dec 20, 2017

@waldenraines - rebased and updated the description.

waldenraines
waldenraines previously approved these changes Dec 20, 2017
Copy link

@waldenraines waldenraines left a comment

Choose a reason for hiding this comment

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

LGTM

@jgiardino
Copy link
Contributor

  1. Updated to Log in

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.

  1. Do you mean to remove the FormControlFeedback from all the stories?
    If so, I think I should remove this component from patternfly-react so it can be consumed from react-bootstrap or just add it as a normal markup. So we won't supply components which are not consistent with patternfly.

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.

  1. I found the spinner example here (at the bottom of this page): http://www.patternfly.org/pattern-library/forms-and-controls/buttons-on-forms/

Thanks for sharing that example. I have totally overlooked it. :-) This one looks great to me!

@priley86
Copy link
Member

priley86 commented Dec 20, 2017

@sharvit please rebase once more, apologies. #137 changes comma settings.

@sharvit
Copy link
Contributor Author

sharvit commented Dec 20, 2017

@priley86 - rebased

Thanks for the feedback @jgiardino

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.

That's fine, updated 👍

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.

Agree, done.

What do you think about exporting the VerticalFormField, HorizontalFormField and the InlineFormField?
tbh, i got mixed feelings about it. It gives easier way for implementing forms, but it takes away flexibility.

@priley86
Copy link
Member

priley86 commented Dec 20, 2017

@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...

@sharvit
Copy link
Contributor Author

sharvit commented Dec 21, 2017

@priley86 I agree with you here, will wait to see how is the experience of using those components.

@jgiardino
Copy link
Contributor

No objections from me, @priley86.
Thanks for the updates, @sharvit!

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!

Copy link
Member

@jeff-phillips-18 jeff-phillips-18 left a comment

Choose a reason for hiding this comment

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

LGTM

@sharvit
Copy link
Contributor Author

sharvit commented Jan 7, 2018

@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?
Would suggest writing some API around the documentation urls so it will be easier to upgrade next time.

@priley86
Copy link
Member

priley86 commented Jan 8, 2018

@sharvit @jgiardino this should be easy enough. We can write a helper and update all links in a subsequent PR.

@jeff-phillips-18 jeff-phillips-18 merged commit 6321564 into patternfly:master Jan 8, 2018
@sharvit sharvit deleted the feature/forms branch January 8, 2018 12:47
@cdcabrera cdcabrera mentioned this pull request Jan 8, 2018
@priley86
Copy link
Member

priley86 commented Jan 8, 2018

opened #154 to address the documentation links

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.

Password Input Field Support Form Validation States Component - Horizontal Forms

5 participants