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(List): handler for <List selection/> #1530

Merged
merged 10 commits into from
Apr 3, 2017

Conversation

josie11
Copy link
Contributor

@josie11 josie11 commented Mar 30, 2017

To address the enhancement requested in issue #1301.

  • onItemClick can be passed to List when there are no children; for click handling of list items
    <List content={['item1', 'item2', 'item3']} onItemClick={console.log} />

  • ListItem has click handler for passing list item props.

@codecov-io
Copy link

codecov-io commented Mar 30, 2017

Codecov Report

Merging #1530 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1530      +/-   ##
==========================================
- Coverage   99.74%   99.74%   -0.01%     
==========================================
  Files         141      141              
  Lines        2384     2381       -3     
==========================================
- Hits         2378     2375       -3     
  Misses          6        6
Impacted Files Coverage Δ
src/elements/List/ListContent.js 100% <100%> (ø) ⬆️
src/elements/List/ListItem.js 100% <100%> (ø) ⬆️
src/elements/List/List.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 754416e...e2a69ae. Read the comment docs.

Copy link
Member

@levithomason levithomason left a comment

Choose a reason for hiding this comment

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

Changes look great. We just need to add some tests. Looking, there are a few things I'd like to fix with tests in this area, so I'll push those shortly.

@levithomason
Copy link
Member

I've converted the List/ListItem components into classes. This is due to the click handlers. We don't want to re-create them on every render, so they need to live on the class instance.

I then added a couple tests for the click handler. I'm a little torn on the differences between onItemClick here and in Menu.js (where I added a TODO).

The menu preserves the user's menu item onClick handler and calls it along with the onItemClick handler. I'll have to come back here and make some decisions on how this should work.

Thoughts?

@josie11
Copy link
Contributor Author

josie11 commented Mar 31, 2017

Hm. It may be useful to preserve onClick while using onItemClick if you have a ListItem(s)/MenuItem(s) that you want to have any additional behavior beyond onItemClick; I did not consider that. But perhaps logic inside the passed onItemClick can deal with customizing the behavior depending on the menu item that is clicked (if|else/switch).

_.invoke(predefinedProps, 'onClick', e, itemProps)
_.invoke(this.props, 'onItemClick', e, itemProps)
},
})
Copy link
Member

Choose a reason for hiding this comment

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

I've updated handlers there like in #1428. I also removed index there, I think we don't really need it there.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, the Menu only needs it as there is an activeIndex.


onClick.should.have.been.calledOnce()
onClick.should.have.been.calledWithMatch(event, callbackData)
Copy link
Member

Choose a reason for hiding this comment

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

I also updated test, now it tests that item handler fired too

Copy link
Member

@layershifter layershifter left a comment

Choose a reason for hiding this comment

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

@levithomason I've rebased with master and performed some updates in handlers and tests. Also added some missing tests for shorthand factories. I think we ready for review 👍

@josie11 Thanks for PR 👍

expect(click).to.not.throw()
})

it('is called with (e, { index, ...itemProps }) when clicked', () => {
Copy link
Member

Choose a reason for hiding this comment

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

No more index here.

@levithomason
Copy link
Member

Looks great, made one update to a test description. Will merge tomorrow (3am here)!

@levithomason levithomason changed the title feat(List) handler for <List selection/> feat(List): handler for <List selection/> Apr 3, 2017
@levithomason levithomason merged commit bf09457 into Semantic-Org:master Apr 3, 2017
@levithomason
Copy link
Member

Released in [email protected].

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.

4 participants