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(Dropdown): Pass object with name to onChange #581

Merged
merged 2 commits into from Oct 3, 2016

Conversation

Projects
None yet
3 participants
@jeffcarbs
Member

jeffcarbs commented Oct 2, 2016

Fixes #218

This is a breaking change but I think it's worth it. There's currently no easy way to get the name of the Dropdown in the onChange event. Normally in an onChange the event.target will be the input that received the change. So in a form, for example, you can attach the same change handler to multiple elements and use the event to determine the appropriate field/value to update.

For Dropdown that's not the case - it will likely be a DropdownItem.

The Checkbox is an example of a component that faces a similar issue regarding the target. In that component, the onClick and onChange are both called with { name, value, checked }. This PR updates Dropdown to be called with { name, value }.

I also cleaned up the naming of our internal event handler to be handleChange. The pattern of this project seems to be:

  • handle<event> for our internal handlers
  • on<event> is a prop that the user passes in and is called by our internal handlers if set.
@jeffcarbs

This comment has been minimized.

Show comment
Hide comment
@jeffcarbs

jeffcarbs Oct 2, 2016

Member

This also uncomments the test referenced in https://github.com/TechnologyAdvice/stardust/issues/218. I grepped for all of our event handlers and it seems that they are all called with the event first so I think that can be closed with this PR.

Member

jeffcarbs commented Oct 2, 2016

This also uncomments the test referenced in https://github.com/TechnologyAdvice/stardust/issues/218. I grepped for all of our event handlers and it seems that they are all called with the event first so I think that can be closed with this PR.

@jeffcarbs

This comment has been minimized.

Show comment
Hide comment
@jeffcarbs

jeffcarbs Oct 2, 2016

Member

Actually realized that onAddItem wasn't receiving an event first. This updates the arguments so the event is passed first then { name, value }.

So two breaking changes in this PR, both in Dropdown:

-onChange(e, value)
+onChange(e, { name, value })

-onAddItem(value)
+onAddItem(e, { name, value })
Member

jeffcarbs commented Oct 2, 2016

Actually realized that onAddItem wasn't receiving an event first. This updates the arguments so the event is passed first then { name, value }.

So two breaking changes in this PR, both in Dropdown:

-onChange(e, value)
+onChange(e, { name, value })

-onAddItem(value)
+onAddItem(e, { name, value })
@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Oct 2, 2016

Current coverage is 99.62% (diff: 100%)

Merging #581 into master will decrease coverage by <.01%

@@             master       #581   diff @@
==========================================
  Files           118        118          
  Lines          1870       1865     -5   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits           1863       1858     -5   
  Misses            7          7          
  Partials          0          0          

Powered by Codecov. Last update 2c144d1...1ad8abd

codecov-io commented Oct 2, 2016

Current coverage is 99.62% (diff: 100%)

Merging #581 into master will decrease coverage by <.01%

@@             master       #581   diff @@
==========================================
  Files           118        118          
  Lines          1870       1865     -5   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits           1863       1858     -5   
  Misses            7          7          
  Partials          0          0          

Powered by Codecov. Last update 2c144d1...1ad8abd

@levithomason

This comment has been minimized.

Show comment
Hide comment
@levithomason

levithomason Oct 2, 2016

Member

Cool, looks good. I think we should always pass (e, data) where data is an object. Just to keep it consistent and extensible.

I'll try to review/merge/ship this tonight.

Member

levithomason commented Oct 2, 2016

Cool, looks good. I think we should always pass (e, data) where data is an object. Just to keep it consistent and extensible.

I'll try to review/merge/ship this tonight.

@levithomason levithomason changed the title from feat(Dropdown): Pass name to onChange to feat(Dropdown): Pass object with name to onChange Oct 3, 2016

@levithomason levithomason merged commit 54c6906 into master Oct 3, 2016

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 99.62%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +0.37% compared to 2c144d1
Details

@levithomason levithomason deleted the feature/dropdown-on-change branch Oct 3, 2016

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