-
Notifications
You must be signed in to change notification settings - Fork 11
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
Add Tabs component #85
Conversation
src/components/Tabs/Tabs.jsx
Outdated
selectedIndex: index | ||
}); | ||
if (this.props.onClick) { | ||
this.props.onClick(index); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm wondering if we can somehow give a key
to each tab, so that what's passed to the handler is like, 'my_tab' and 'another_tab' instead of 0
or 1
. that would be less fragile if we were to add new tabs or reorder existing ones.
the style & component in general look good, however see my comment about using some sort of |
whoops didn't mean to close, sorry! |
src/components/Tabs/Tabs.jsx
Outdated
constructor(props) { | ||
super(props); | ||
|
||
let selectedIndex = _.findIndex(props.items, item => item.selected); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: maybe the lodash shorthand exists for this: _.findIndex(props.items, 'selected')
I made this pretty minimal for now — just a way to pass an array of strings and a single callback for all of them that's being passed an index of a selected tab.
Added support for the keyboard as well since it's not too complicated although not sure how important that is at the moment. It'd be nice to add ARIA roles as well but that'd probably be overkill right now.