Feat: Initial implementation of Tabs component #131
Conversation
|
Couple of comments I think this could use the .tab-content, .tab-pane classes and .active from http://v4-alpha.getbootstrap.com/components/navs/#using-data-attributes (fade option could be part of a different pr). I should have mentioned that in GitHub issue. Also, instead of iterating through children and setting state, you could use context to simplify the TabContent component https://facebook.github.io/react/docs/context.html. Ex: add getChildContext to TabContent, return {activeTab: this.props.activeTab}. In TabPane, it can accept a 2nd argument, context, from there you can compare props.tabId to context.activeTab and add the .active class if they are equal. For coverage, there's a folder in test directory called coverage and somewhere inside is a root index.html. I usually open that in a browser and sort by missing coverage Thanks for tackling this! feel free to ping me here or in slack if you need anything. The link to it is in contributing.md |
|
@eddywashere Will work on this weekend. Just one minor question, react docs mentions using context only when it is absolutely necessary and try to avoid it as much as possible. Is there any specific reason why we should use contextTypes for communicating with children? The props seemed to be simple. Is there any specific reason we should use contextTypes? (Sorry if my question is a little silly, I am relatively new to react :)) |
|
Glad you brought that up. "Using context makes your components more coupled" I interpret that as a positive effect in this case. TabPanes are coupled to TabContent. It's partially to save time passing the same prop to child components. If the API changes we could always switch to cloning children(TabPanes) with the activeTab prop from TabContent. That wouldn't cause a breaking change and seems like an easy win for keeping things simple. |
…+ pushes code coverage to 100%
|
@eddywashere That was super useful! Have updated the PR. Please review and let me know I have missed anything. |
| }; | ||
| } | ||
| componentWillReceiveProps(nextProps) { | ||
| this.setState({ |
There was a problem hiding this comment.
you should check if nextProps.activeTab !== this.props.activeTab before using setState
There was a problem hiding this comment.
Not sure why github is not collapsing or updating this comment but this is fixed
…ames out to const variables from jsx
ajainarayanan
force-pushed
the
feature/Tabs
branch
from
b5bd11b
to
7d20b8d
Compare
|
thanks a ton for working through this @ajainarayanan! I'll squash and merge via github. |
| it('should show no active tabs if active tab id is unknown', () => { | ||
| let tab1 = mount(<TabsExample />); | ||
| const instance = tab1.instance(); | ||
| expect(instance instanceof TabsExample).toBe(true); |
There was a problem hiding this comment.
are you testing the example?.... as in your are not testing the actual Tab component.....
There was a problem hiding this comment.
The TabExample is a wrapper around the Tab. The aim in this test is to have a click initiated from the wrapper that toggles the TabContent (in this case an invalid case). The clicking and managing ActiveTab is not in the scope of the Tab component. The test still focuses on testing the Tab and not the TabsExample though.
There was a problem hiding this comment.
It's just this except tests the example directly, seems pointless.
PR for #72
Note: