-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Using NavigableContainer to make a TabPanel for Inserter #3281
Changes from 52 commits
51f344a
fc71703
fadd1a2
51d9e79
3722ef3
bc2ada7
d02c973
42fa2df
378ad44
c000663
203a45c
bccc2b8
ef72bc3
3b08fa8
1bc5225
cfbd768
642f24c
c20a30b
bc9d947
fa5478c
9876bd3
f6d6cd1
3b8346c
0f6a983
c45bbd4
e93b4bf
c1d815a
b9b4a15
749e0f0
ba8c056
85fbf54
b38d8b7
0dd9844
eab793b
387c790
ea87372
b2ccb7c
cb046e3
e398448
f946a80
48ecdc1
ffdca17
675ca21
65d97e6
e12f6fd
f90b57f
81d490f
90e40bd
de08428
1871b9d
bdfc7a2
495b0cb
dddaae0
ac75a8a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,100 @@ | ||
TabPanel | ||
======= | ||
|
||
TabPanel is a React component to render an ARIA-compliant TabPanel. It has two sections: a list of tabs, and the view to show when tabs are chosen. When the list of tabs gets focused, the active tab gets focus (the first tab if there isn't one already). Use the arrow keys to navigate between tabs AND select the newly focused tab at the same time. | ||
|
||
TabPanel is a Function-as-Children component. The function takes `tabName` as an argument. | ||
|
||
## Usage | ||
|
||
Renders a TabPanel with each tab representing a paragraph with its title. | ||
|
||
```jsx | ||
|
||
import { TabPanel } from '@wordpress/components'; | ||
|
||
const onSelect = ( tabName ) => { | ||
console.log( 'Selecting tab', tabName ); | ||
}; | ||
|
||
function MyTabPanel() { | ||
return ( | ||
<TabPanel className="my-tab-panel" | ||
activeClass="active-tab" | ||
onSelect={ onSelect } | ||
tabs={ [ | ||
{ | ||
name: 'tab1', | ||
title: 'Tab 1', | ||
className: 'tab-one', | ||
}, | ||
{ | ||
name: 'tab2', | ||
title: 'Tab 2', | ||
className: 'tab-two', | ||
}, | ||
] }> | ||
{ | ||
( tabName ) => { | ||
return <p>${ tabName }</p>; | ||
} | ||
} | ||
</TabPanel> | ||
) | ||
} | ||
``` | ||
|
||
## Props | ||
|
||
The component accepts the following props: | ||
|
||
### className | ||
|
||
The class to give to the outer container for the TabPanel | ||
|
||
- Type: `String` | ||
- Required: No | ||
- Default: '' | ||
|
||
### orientation | ||
|
||
The orientation of the tablist (`vertical` or `horizontal`) | ||
|
||
- Type: `String` | ||
- Required: No | ||
- Default: `horizontal` | ||
|
||
### onSelect | ||
|
||
The function called when a tab has been selected. It is passed the `tabName` as an argument. | ||
|
||
- Type: `Function` | ||
- Required: No | ||
- Default: `noop` | ||
|
||
### tabs | ||
|
||
A list of tabs where each tab is defined by an object with the following fields: | ||
|
||
1. name: String. Defines the key for the tab | ||
2. title: String. Defines the translated text for the tab | ||
3. className: String. Defines the class to put on the tab. | ||
|
||
- Type: Array | ||
- Required: Yes | ||
|
||
### activeClass | ||
|
||
The class to add to the active tab | ||
|
||
- Type: `String` | ||
- Required: No | ||
- Default: `is-active` | ||
|
||
### children | ||
|
||
A function which renders the tabviews given the selected tab. The function is passed a `tabName` as an argument. | ||
The element to which the tooltip should anchor. | ||
|
||
- Type: (`String`) => `Element` | ||
- Required: Yes | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we make this optional providing a default value, returning the tabTitle ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh. I originally thought this was used to render the tab links and not the content of the selected tab which makes the purpose of the In this case, I think we could benefit from provide an extra There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Each At the moment, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you're right. Since the |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,94 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
import { partial, noop, omit } from 'lodash'; | ||
|
||
/** | ||
* WordPress dependencies | ||
*/ | ||
// import './style.scss'; | ||
import { Component } from '@wordpress/element'; | ||
|
||
/** | ||
* Internal Dependencies | ||
*/ | ||
import { default as withInstanceId } from '../higher-order/with-instance-id'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can be grabbed as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, when I try this, my script fails at runtime. Is there something wrong with my setup? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, interesting. Nothing wrong, but webpack isn't too happy. Other modules in Aside, that section should be broken down into two: WordPress dependencies and internal dependencies. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You don't need import withInstanceId from '../higher-order/with-instance-id'; The default export is inferred when assigning outside the curly braces. |
||
import { NavigableMenu } from '../navigable-container'; | ||
|
||
const TabButton = ( { tabId, onClick, children, selected, ...rest } ) => { | ||
return <button role="tab" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: might want to use parens around the element here: return (
<element />
); |
||
tabIndex={ selected ? null : -1 } | ||
aria-selected={ selected } | ||
id={ tabId } | ||
onClick={ onClick } | ||
{ ...rest } | ||
> | ||
{ children } | ||
</button>; | ||
}; | ||
|
||
class TabPanel extends Component { | ||
constructor() { | ||
super( ...arguments ); | ||
|
||
this.handleClick = this.handleClick.bind( this ); | ||
this.onNavigate = this.onNavigate.bind( this ); | ||
|
||
this.state = { | ||
selected: this.props.tabs.length > 0 ? this.props.tabs[ 0 ].name : null, | ||
}; | ||
} | ||
|
||
handleClick( tabKey ) { | ||
const { onSelect = noop } = this.props; | ||
this.setState( { | ||
selected: tabKey, | ||
} ); | ||
onSelect( tabKey ); | ||
} | ||
|
||
onNavigate( childIndex, child ) { | ||
child.click(); | ||
} | ||
|
||
render() { | ||
const { selected } = this.state; | ||
const { instanceId, tabs, activeClass = 'is-active', className, orientation = 'horizontal' } = this.props; | ||
|
||
const selectedTab = tabs.find( ( t ) => t.name === selected ); | ||
const selectedId = instanceId + '-' + selectedTab.name; | ||
|
||
return ( | ||
<div> | ||
<NavigableMenu | ||
role="tablist" | ||
orientation={ orientation } | ||
onNavigate={ this.onNavigate } | ||
className={ className } | ||
> | ||
{ tabs.map( ( tab ) => | ||
<TabButton className={ `${ tab.className } ${ tab.name === selected ? activeClass : '' }` } | ||
tabId={ instanceId + '-' + tab.name } | ||
aria-controls={ instanceId + '-' + tab.name + '-view' } | ||
selected={ tab.name === selected } | ||
key={ tab.name } | ||
onClick={ partial( this.handleClick, tab.name ) } | ||
> | ||
{ tab.title } | ||
</TabButton> ) | ||
} | ||
</NavigableMenu> | ||
{ | ||
selectedTab && | ||
<div aria-labelledby={ selectedId } | ||
role="tabpanel" | ||
id={ selectedId + '-view' }> | ||
{ this.props.children( selectedTab.name ) } | ||
</div> | ||
} | ||
</div> | ||
); | ||
} | ||
} | ||
|
||
export default withInstanceId( TabPanel ); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,91 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
import { mount } from 'enzyme'; | ||
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import TabPanel from '../'; | ||
|
||
describe( 'TabPanel', () => { | ||
describe( 'basic rendering', () => { | ||
it( 'should render a tabpanel, and clicking should change tabs', () => { | ||
const wrapper = mount( | ||
<TabPanel className="test-panel" | ||
activeClass="active-tab" | ||
tabs={ | ||
[ | ||
{ | ||
name: 'alpha', | ||
title: 'Alpha', | ||
className: 'alpha', | ||
}, | ||
{ | ||
name: 'beta', | ||
title: 'Beta', | ||
className: 'beta', | ||
}, | ||
{ | ||
name: 'gamma', | ||
title: 'Gamma', | ||
className: 'gamma', | ||
}, | ||
] | ||
} | ||
> | ||
{ | ||
( tabName ) => { | ||
return <p tabIndex="0" className={ tabName + '-view' }>{ tabName }</p>; | ||
} | ||
} | ||
</TabPanel> | ||
); | ||
|
||
const alphaTab = wrapper.find( 'button.alpha' ); | ||
const betaTab = wrapper.find( 'button.beta' ); | ||
const gammaTab = wrapper.find( 'button.gamma' ); | ||
|
||
const getAlphaView = () => wrapper.find( 'p.alpha-view' ); | ||
const getBetaView = () => wrapper.find( 'p.beta-view' ); | ||
const getGammaView = () => wrapper.find( 'p.gamma-view' ); | ||
|
||
const getActiveTab = () => wrapper.find( 'button.active-tab' ); | ||
const getActiveView = () => wrapper.find( 'div[role="tabpanel"]' ); | ||
|
||
expect( getActiveTab().text() ).toBe( 'Alpha' ); | ||
expect( getAlphaView().length ).toBe( 1 ); | ||
expect( getBetaView().length ).toBe( 0 ); | ||
expect( getGammaView().length ).toBe( 0 ); | ||
expect( getActiveView().text() ).toBe( 'alpha' ); | ||
|
||
betaTab.simulate( 'click' ); | ||
expect( getActiveTab().text() ).toBe( 'Beta' ); | ||
expect( getAlphaView().length ).toBe( 0 ); | ||
expect( getBetaView().length ).toBe( 1 ); | ||
expect( getGammaView().length ).toBe( 0 ); | ||
expect( getActiveView().text() ).toBe( 'beta' ); | ||
|
||
betaTab.simulate( 'click' ); | ||
expect( getActiveTab().text() ).toBe( 'Beta' ); | ||
expect( getAlphaView().length ).toBe( 0 ); | ||
expect( getBetaView().length ).toBe( 1 ); | ||
expect( getGammaView().length ).toBe( 0 ); | ||
expect( getActiveView().text() ).toBe( 'beta' ); | ||
|
||
gammaTab.simulate( 'click' ); | ||
expect( getActiveTab().text() ).toBe( 'Gamma' ); | ||
expect( getAlphaView().length ).toBe( 0 ); | ||
expect( getBetaView().length ).toBe( 0 ); | ||
expect( getGammaView().length ).toBe( 1 ); | ||
expect( getActiveView().text() ).toBe( 'gamma' ); | ||
|
||
alphaTab.simulate( 'click' ); | ||
expect( getActiveTab().text() ).toBe( 'Alpha' ); | ||
expect( getAlphaView().length ).toBe( 1 ); | ||
expect( getBetaView().length ).toBe( 0 ); | ||
expect( getGammaView().length ).toBe( 0 ); | ||
expect( getActiveView().text() ).toBe( 'alpha' ); | ||
} ); | ||
} ); | ||
} ); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,86 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
import { isEqual } from 'lodash'; | ||
|
||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { Component } from '@wordpress/element'; | ||
import { NavigableMenu } from '@wordpress/components'; | ||
import { BlockIcon } from '@wordpress/blocks'; | ||
|
||
function deriveActiveBlocks( blocks ) { | ||
return blocks.filter( ( block ) => ! block.disabled ); | ||
} | ||
|
||
export default class InserterGroup extends Component { | ||
constructor() { | ||
super( ...arguments ); | ||
|
||
this.onNavigate = this.onNavigate.bind( this ); | ||
|
||
this.activeBlocks = deriveActiveBlocks( this.props.blockTypes ); | ||
this.state = { | ||
current: this.activeBlocks.length > 0 ? this.activeBlocks[ 0 ].name : null, | ||
}; | ||
} | ||
|
||
componentWillReceiveProps( nextProps ) { | ||
if ( ! isEqual( this.props.blockTypes, nextProps.blockTypes ) ) { | ||
this.activeBlocks = deriveActiveBlocks( nextProps.blockTypes ); | ||
// Try and preserve any still valid selected state. | ||
const current = this.activeBlocks.find( block => block.name === this.state.current ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we provide a runtime warning in dev env? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Per #746 (comment), I think eventually we'll just want to pull in the polyfill, ideally once Babel 7.x lands. |
||
if ( ! current ) { | ||
this.setState( { | ||
current: this.activeBlocks.length > 0 ? this.activeBlocks[ 0 ].name : null, | ||
} ); | ||
} | ||
} | ||
} | ||
|
||
renderItem( block ) { | ||
const { current } = this.state; | ||
const { selectBlock, bindReferenceNode } = this.props; | ||
const disabled = block.disabled; | ||
return ( | ||
<button | ||
role="menuitem" | ||
key={ block.name } | ||
className="editor-inserter__block" | ||
onClick={ selectBlock( block.name ) } | ||
ref={ bindReferenceNode( block.name ) } | ||
tabIndex={ current === block.name || disabled ? null : '-1' } | ||
onMouseEnter={ ! disabled ? this.props.showInsertionPoint : null } | ||
onMouseLeave={ ! disabled ? this.props.hideInsertionPoint : null } | ||
disabled={ disabled } | ||
> | ||
<BlockIcon icon={ block.icon } /> | ||
{ block.title } | ||
</button> | ||
); | ||
} | ||
|
||
onNavigate( index ) { | ||
const { activeBlocks } = this; | ||
const dest = activeBlocks[ index ]; | ||
if ( dest ) { | ||
this.setState( { | ||
current: dest.name, | ||
} ); | ||
} | ||
} | ||
|
||
render() { | ||
const { labelledBy, blockTypes } = this.props; | ||
|
||
return <NavigableMenu | ||
className="editor-inserter__category-blocks" | ||
orientation="vertical" | ||
aria-labelledby={ labelledBy } | ||
cycle={ false } | ||
onNavigate={ this.onNavigate }> | ||
{ blockTypes.map( this.renderItem, this ) } | ||
</NavigableMenu>; | ||
} | ||
} |
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.
Should this be optional, providing a default value?
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 guess so. I'm new to the class naming conventions. What would you propose?
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.
is-active
?