Conversation
cjcenizal
requested review from
arkwright,
bevacqua,
cchaos,
chandlerprall and
snide
cjcenizal
force-pushed
the
tabbed-content
branch
2 times, most recently
from
42ce61e to
4540cf6
Compare
snide
left a comment
snide
left a comment
There was a problem hiding this comment.
Styling and function look proper. Thanks for passing down the size prop as well. Might want to doc your props.
arkwright
left a comment
arkwright
left a comment
There was a problem hiding this comment.
Looking great! I left a few semantic considerations.
| } = this.props; | ||
|
|
||
| // Allow the consumer to control tab selection. | ||
| const selectedTab = externalSelectedTab || this.state.selectedTab |
There was a problem hiding this comment.
Just thinking out loud here, but if I was searching for use of the selectedTab prop it would trip me up to see it renamed to externalSelectedTab. I think it might be easier to figure out what's going on to write this as this.props.selectedTab. I guess it depends how heavily we want to lean on the implicit statement that props are externally specified.
There was a problem hiding this comment.
I definitely see what you're saying. We have to destructure this prop anyway so that we can pass ...rest onto the rendered root element, which would also be confusing because we would destructure it, declare it as unused (via an eslint comment), and then refer to it anyway via this.props. I'm going to be lazy and leave this as-is for now. 😄
| return ( | ||
| <div className={className} {...rest}> | ||
| <EuiTabs size={size}> | ||
| { |
There was a problem hiding this comment.
Looks like the indentation for these braces is off a bit.
| ...tabProps, | ||
| onClick: () => this.onTabClick(tab), | ||
| isSelected: tab === selectedTab, | ||
| 'aria-controls': `${this.rootId}-${id}`, |
|
|
||
| // Mock the htmlIdGenerator to generate predictable ids for snapshot tests | ||
| jest.mock('../../../services/accessibility/html_id_generator', () => ({ | ||
| htmlIdGenerator: () => { return () => 42; }, |
| }); | ||
|
|
||
| describe('selectedTab', () => { | ||
| test('is rendered', () => { |
There was a problem hiding this comment.
I understand what this means, but it might be unclear for the next person what is being rendered in this case.
cjcenizal
force-pushed
the
tabbed-content
branch
from
6b9a5bc to
f257d3a
Compare
| /** | ||
| * Each tab needs an id property, so we can associate it with its panel for accessibility | ||
| */ | ||
| tabs: PropTypes.arrayOf(PropTypes.shape({ id: PropTypes.string.isRequired })).isRequired, |
There was a problem hiding this comment.
name should be required as it's the user-readable label on the tab
content should be required because this component combines tab+content
| this.rootId = makeId(); | ||
|
|
||
| this.state = { | ||
| selectedTab: selectedTab || initialSelectedTab || tabs[0], |
There was a problem hiding this comment.
if this component is controlled it should not track its own state independently, instead setting this to null
| onTabClick(selectedTab) | ||
| } | ||
|
|
||
| this.setState({ selectedTab }) |
There was a problem hiding this comment.
likewise, if this component is controlled it should not store the selected tab in its own state
|
Thanks @chandlerprall! I've addressed your feedback. Can you take another look? |
|
jenkins test this |
1 similar comment
|
jenkins test this |
5 participants
This component makes it easy to connect tabs with the content they display. You can either delegate state management to the component or provide props to control its state. Thanks @arkwright for building the initial version of this!