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): update to v1 API #400

Merged
merged 13 commits into from
Sep 1, 2016
Merged

Conversation

levithomason
Copy link
Member

@levithomason levithomason commented Aug 16, 2016

Fixes #247 Remove jQuery Dependency
Fixes #432 Hoist propTypes for all wrapper components

This PR addresses the final item in removing jQuery. Since the last component is the Form, this PR also updates the Form to the v1 API and handles validation.

Reviewing

Try out the Form docs first and look at the code snippets to get a good feel for the API. Then, focus on src/collections/Form/*.js. Most of the code here is just docs, tests, and updating old Form usages.

Also, have a look at the line comments I left below.

Breaking Changes

Form

  • The entire component API has been replaced, see docs.
  • Removed jQuery validation. React friendly validation will be added soon.

Checkbox

Confusion between radio, checkbox, slider, toggle, and various combinations thereof have been addressed. See the docs for updated usage.

inputType & type

The inputType prop has been removed, set type instead.

The type prop is no longer used to set visual SUI type ("slider", "toggle", "checkbox", "radio"). It now controls the type of the html input.

Radio

inputType & type

See Checkbox

Exclusive Groups

Radio groups (multiple radios with the same name) must be controlled components. Previously we relied on the browser to manage radio group state and used refs to read the checked state. Reading from the DOM is an antipattern.

@levithomason levithomason mentioned this pull request Aug 16, 2016
24 tasks
@brsanthu
Copy link
Contributor

@levithomason any idea when this might be merged?

@levithomason
Copy link
Member Author

levithomason commented Aug 16, 2016

I'm actively working on this, though I suspect it will be a few days of work. I'm considering shipping the API without Form validation first. Then, shipping the validation later. This could save a substantial amount of time.

This is because validation is something we're still tossing ideas around on. Input highly encouraged!

@brsanthu
Copy link
Contributor

I think shipping without validation and adding that later is good idea. When validations are added, it would be progressive and (I hope) will not have any api impact.

@levithomason
Copy link
Member Author

The basic API should not change when validation is added.

@levithomason levithomason changed the title Form: update to v1 API, add validation, remove jquery Form: update to v1 API and remove jquery Aug 19, 2016
@levithomason
Copy link
Member Author

Confirmed, validation will be done separately. See #407 for that discussion.

@codecov-io
Copy link

codecov-io commented Aug 21, 2016

Current coverage is 98.30% (diff: 98.80%)

Merging #400 into master will increase coverage by 1.57%

@@             master       #400   diff @@
==========================================
  Files            91         98     +7   
  Lines          1312       1413   +101   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1269       1389   +120   
+ Misses           43         24    -19   
  Partials          0          0          

Powered by Codecov. Last update 8ac4291...2503a44

@levithomason levithomason force-pushed the feature/form-without-jquery branch 2 times, most recently from 06380b2 to 98d029c Compare August 21, 2016 23:02
/>
>
<Message warning>
Radios in a group must be
Copy link
Member

Choose a reason for hiding this comment

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

&nbsp; should probably be after the be? Otherwise it doesn't achieve anything different than a {' ' }.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not aware of the differences between those, though the spaced string did give issues elsewhere.

Since space is removed between components that are separated by a new line, and the component's content should always be one space away from adjacent text, I've opted to include &nbsp; inside of components. This way, no matter where the components are moved the spacing is correct without having to add/remove spacing elements every time you move the component.

@dvdzkwsk
Copy link
Member

Looks awesome, that was a ton of work, so massive props on getting it so solid.

My only real hold up is the conflating of the type property for the Radio component. If that can be resolved you have my emoji.

@levithomason levithomason merged commit 8d53dad into master Sep 1, 2016
@levithomason levithomason deleted the feature/form-without-jquery branch September 1, 2016 20:20
@levithomason
Copy link
Member Author

Released in v0.40.0

/cc @brsanthu

@brsanthu
Copy link
Contributor

brsanthu commented Sep 2, 2016

thanks for the great release @levithomason. This is the major piece. Is this end of Jquery in stardust?

@levithomason
Copy link
Member Author

levithomason commented Sep 2, 2016

This is indeed the end of jQuery in Stardust. There are no traces of jQuery in this library any longer. See #247.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants