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(form): add forms and form element components #67

Merged
merged 3 commits into from
Aug 6, 2016

Conversation

TheSharpieOne
Copy link
Member

@TheSharpieOne TheSharpieOne commented Jul 29, 2016

#50

Forms and form element components as well as docs and tests including:

  • Form
  • FormGroup
  • FormFeedback
  • FormText
  • Input
  • Label

What this doesn't include for forms:

  • input-group and input-group-addon
  • something about grouping radio and checks (this seems like something twbs is changing in v4 (new .form-check vs .radio and .check) half of their docs use or the other)
  • custom forms (Is this new to bs4, don't think I have seen this before)
  • fieldsets (doesn't seem like it would be useful since everything would just pass right though <Fieldset disabled> -> <fieldset disabled>

Also, these are the basic building block components, there are no helper/wrapping components (like a single component which will build out a form group with a label and an input for you.) The helper/wrapping components can utilize these, if you determine this library should have such components (in which case, let me know and I can add them).

I did the best I could with docs, there are a lot of examples which shows everything, but not everything has a dedicated section.

@coveralls
Copy link

coveralls commented Jul 29, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 4d77d28 on TheSharpieOne:feature/forms into 257f378 on reactstrap:master.

@byennen
Copy link

byennen commented Jul 31, 2016

👍

@eddywashere
Copy link
Member

Apologies for the delay in reviewing this. It was a bigger pr and I didn't have a large block of time until today.

  • I think the examples are sufficient until it becomes clearer in bootstrap docs. either way it could be improved in another pr
  • Custom forms is definitely new.
  • Just read up on fieldset caveats in the docs. That could have been the default tag for FormGroup. Seems great for modern browsers, but might be more trouble than it's worth.

re: "Also, these are the basic building block components, there are no helper/wrapping components"

I usually put together something like that for projects I'm working on. ex: forms, I usually have a formgroup, error and validation wrapped components (tailored to app ux). If they exist, they should be part of another repo like reactstrap-helpers. It would be a good reference for people learning how to piece things together.

};

const defaultProps = {
tag: 'p',
Copy link
Member

Choose a reason for hiding this comment

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

I wasn't sure why this would ever happen but then I remembered the static text class. 👍

@eddywashere
Copy link
Member

A few comments. Great work!

);

return (
<Tag {...attributes} className={classes} htmlFor={htmlFor} />
Copy link
Member

Choose a reason for hiding this comment

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

consider adding {...attributes} after htmlFor={htmlFor}, I could imagine someone not using for and sticking with htmlFor, in which case it would be overridden with a falsey attribute.

@TheSharpieOne
Copy link
Member Author

Sorry about the copy-pasta with the exported component name. I'll clean these up Monday.

As for taking select and textarea out, would it be acceptable to leave them in and create convenience components called Select and Textarea which simply wrap Input to set the type? I believe it will make this project's code a bit cleaner (imho, since then there will not be components which do the similar thing except for output a different tag) and more clear for the dev user (seeing Select instead of Input type="select") while also making it easy if they Inputs are dynamically created (by the dev user via some config and custom things they set up (no need for a switch to determine which component to use, always Input).

@eddywashere
Copy link
Member

Hmmm. That's a fair point. There would still be a switch for type and props assigned. In that case, no need to include Select & Textarea in this pr. If it's a big enough problem it can be addressed in a later pr without a breaking change. Removing it after it existed would be a breaking change.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling b22fe5a on TheSharpieOne:feature/forms into 257f378 on reactstrap:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling cf8196e on TheSharpieOne:feature/forms into 2252f6f on reactstrap:master.

@TheSharpieOne
Copy link
Member Author

/me kicks travis

@coveralls
Copy link

coveralls commented Aug 1, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 2b495ef on TheSharpieOne:feature/forms into 2252f6f on reactstrap:master.

@eddywashere
Copy link
Member

👍

@eddywashere eddywashere merged commit 7c32483 into reactstrap:master Aug 6, 2016
@TheSharpieOne TheSharpieOne deleted the feature/forms branch August 6, 2016 18:15
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.

None yet

4 participants