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

Using NavigableContainer to make a TabPanel for Inserter #3281

Merged
merged 54 commits into from
Nov 14, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
54 commits
Select commit Hold shift + click to select a range
51f344a
navigable container with basic README and skipped tests
ephox-mogran Nov 1, 2017
fc71703
adding more tests for modes
ephox-mogran Nov 1, 2017
fadd1a2
grid navigation for irregular grids
ephox-mogran Nov 2, 2017
51d9e79
fixing id in skipped test
ephox-mogran Nov 2, 2017
3722ef3
replacing other NavigableMenu
ephox-mogran Nov 2, 2017
bc2ada7
using NavigableMenu throughout app
ephox-mogran Nov 2, 2017
d02c973
whitespace
ephox-mogran Nov 2, 2017
42fa2df
accessing DOM (but tests are skipped anyway)
ephox-mogran Nov 2, 2017
378ad44
readme fixes and a bit of cleanup
ephox-mogran Nov 2, 2017
c000663
initial TabPanel
ephox-mogran Nov 2, 2017
203a45c
using TabPanel in inserter
ephox-mogran Nov 2, 2017
bccc2b8
adding role=grid
ephox-mogran Nov 2, 2017
ef72bc3
only talk about results when searching
ephox-mogran Nov 2, 2017
3b08fa8
inserter test fixes
ephox-mogran Nov 2, 2017
1bc5225
testing TabPanel
ephox-mogran Nov 2, 2017
cfbd768
simplifying context detection
ephox-mogran Nov 7, 2017
642f24c
removing initial selector
ephox-mogran Nov 7, 2017
c20a30b
refactoring getInitialFocus
ephox-mogran Nov 7, 2017
bc9d947
Removing initial selector logic altogether
ephox-mogran Nov 7, 2017
fa5478c
Merge remote-tracking branch 'origin/try/general-navigable-component'…
ephox-mogran Nov 7, 2017
9876bd3
set tabindex to -1 of inactive tabs
ephox-mogran Nov 7, 2017
f6d6cd1
separating out blocks list to have state for currently selected
ephox-mogran Nov 7, 2017
3b8346c
BlocksList separation
ephox-mogran Nov 7, 2017
0f6a983
renaming BlocksList to group
ephox-mogran Nov 7, 2017
c45bbd4
remove modes
ephox-mogran Nov 7, 2017
e93b4bf
merging with general-navigable-component simpler version
ephox-mogran Nov 7, 2017
c1d815a
noop for onNavigate
ephox-mogran Nov 7, 2017
b9b4a15
Merge branch 'try/general-navigable-component' into try/tab-component
ephox-mogran Nov 7, 2017
749e0f0
handling tab properly
ephox-mogran Nov 7, 2017
ba8c056
Merge branch 'try/general-navigable-component' into try/tab-component
ephox-mogran Nov 7, 2017
85fbf54
removing skip comments
ephox-mogran Nov 7, 2017
b38d8b7
Updated readme, tests
ephox-mogran Nov 8, 2017
0dd9844
Working tab navigation
ephox-mogran Nov 8, 2017
eab793b
merged with master
ephox-mogran Nov 8, 2017
387c790
merged with master, and allowing role to be passed in
ephox-mogran Nov 8, 2017
ea87372
more comprehensive testing
ephox-mogran Nov 8, 2017
b2ccb7c
unused import
ephox-mogran Nov 8, 2017
cb046e3
removing incorrect export
ephox-mogran Nov 8, 2017
e398448
Merge remote-tracking branch 'origin/try/general-navigable-component'…
ephox-mogran Nov 8, 2017
f946a80
search deeply for tabbable areas in inserter
ephox-mogran Nov 8, 2017
48ecdc1
removing unnecessary props
ephox-mogran Nov 8, 2017
ffdca17
merged with master
ephox-mogran Nov 9, 2017
675ca21
removing stopper and handleRef
ephox-mogran Nov 9, 2017
65d97e6
Merged with master
ephox-mogran Nov 13, 2017
e12f6fd
PR fixes
ephox-mogran Nov 13, 2017
f90b57f
tests
ephox-mogran Nov 13, 2017
81d490f
Merge remote-tracking branch 'origin/master' into try/tab-component
ephox-mogran Nov 13, 2017
90e40bd
defaulting is-active and separating dependencies
ephox-mogran Nov 13, 2017
de08428
using a functional component
ephox-mogran Nov 13, 2017
1871b9d
handling disabled block types
ephox-mogran Nov 14, 2017
bdfc7a2
Instead of filtering out disabled focusables, just never add a tabind…
ephox-mogran Nov 14, 2017
495b0cb
boolean logic error
ephox-mogran Nov 14, 2017
dddaae0
Style fixes & tweaks
mcsf Nov 14, 2017
ac75a8a
InserterMenu: Don't leak arrow key presses
mcsf Nov 14, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions components/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ export { default as Popover } from './popover';
export { default as ResponsiveWrapper } from './responsive-wrapper';
export { default as SandBox } from './sandbox';
export { default as Spinner } from './spinner';
export { default as TabPanel } from './tab-panel';
export { default as Toolbar } from './toolbar';
export { default as Tooltip } from './tooltip';
export { Slot, Fill, Provider as SlotFillProvider } from './slot-fill';
Expand Down
2 changes: 1 addition & 1 deletion components/navigable-container/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { Component } from '@wordpress/element';
import { focus, keycodes } from '@wordpress/utils';

/**
* Module Constants
* Module constants
*/
const { UP, DOWN, LEFT, RIGHT, TAB } = keycodes;

Expand Down
100 changes: 100 additions & 0 deletions components/tab-panel/README.md
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
Copy link
Contributor

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?

Copy link
Contributor Author

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

is-active?


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
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we make this optional providing a default value, returning the tabTitle ?

Copy link
Contributor

Choose a reason for hiding this comment

The 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 TabPanel bigger than I expected.

In this case, I think we could benefit from provide an extra renderTabTitle prop. If we want to add icons or something like this to the TabTitle

Copy link
Contributor Author

@ephox-mogran ephox-mogran Nov 12, 2017

Choose a reason for hiding this comment

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

Each tab already has a title prop. Do you mean specifically allowing that title to be a component rather than text? Or do you mean having a different render for the title possible when the tab is selected. Or do you mean both?

At the moment, tab.title is used directly as a child of button, so it could be any react component.

Copy link
Contributor

@youknowriad youknowriad Nov 13, 2017

Choose a reason for hiding this comment

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

you're right. Since the title is used only to display the tab, you can still pass any component there. But title has a meaning and if in the future, it's used elsewhere, we can't assume that it can be a component. And we should provide a renderTabTitle prop at that moment. Not important right now.

99 changes: 99 additions & 0 deletions components/tab-panel/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
/**
* External dependencies
*/
import { partial, noop } from 'lodash';

/**
* WordPress dependencies
*/
import { Component } from '@wordpress/element';

/**
* Internal dependencies
*/
import { default as withInstanceId } from '../higher-order/with-instance-id';
Copy link
Member

Choose a reason for hiding this comment

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

You don't need { default as ... } syntax here. This can be simplified to:

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 } ) => (
<button role="tab"
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 {
activeClass = 'is-active',
className,
instanceId,
orientation = 'horizontal',
tabs,
} = this.props;

const selectedTab = tabs.find( ( { name } ) => name === selected );
Copy link
Member

@aduth aduth Nov 15, 2017

Choose a reason for hiding this comment

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

Array#find is not polyfilled and is not supported by IE11. This results in crashes in IE11.

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 );
91 changes: 91 additions & 0 deletions components/tab-panel/test/index.js
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' );
} );
} );
} );
88 changes: 88 additions & 0 deletions editor/components/inserter/group.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
/**
* 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 );
Copy link
Member

Choose a reason for hiding this comment

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

Array#find is not polyfilled and is not supported by IE11. This results in crashes in IE11.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we provide a runtime warning in dev env?

Copy link
Member

Choose a reason for hiding this comment

The 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;
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>
);
}
}
Loading