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.

Sign up for GitHub

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

Components: Improve ToolbarButton #18931

Merged
merged 12 commits into from
Jan 7, 2020
Merged

Components: Improve ToolbarButton #18931

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
badb51d
Initial refactor on ToolbarButton
diegohaz Dec 5, 2019
31dcf58
Update storyshots
diegohaz Dec 5, 2019
e5110b1
Update ToolbarButton styles
diegohaz Dec 5, 2019
474aba5
Merge branch 'master' into update/toolbar-button
diegohaz Dec 11, 2019
da487d9
Merge branch 'master' into update/toolbar-button
diegohaz Dec 14, 2019
b3bf911
Update snapshots
diegohaz Dec 14, 2019
7779fa7
Remove unnecessary styles for components-toolbar-button
diegohaz Dec 15, 2019
0643f3c
Merge branch 'master' into update/toolbar-button
diegohaz Dec 21, 2019
482e28f
Merge branch 'master' into update/toolbar-button
diegohaz Dec 26, 2019
aeca4d6
Update Toolbar Button imports
diegohaz Dec 26, 2019
b3ef374
Refactors
diegohaz Dec 26, 2019
83aeceb
Merge branch 'master' into update/toolbar-button
diegohaz Jan 6, 2020
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
2 changes: 2 additions & 0 deletions packages/components/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,8 @@ export { default as ToggleControl } from './toggle-control';
export { default as Toolbar } from './toolbar';
export { default as ToolbarButton } from './toolbar-button';
export { default as ToolbarGroup } from './toolbar-group';
export { default as __experimentalToolbarIconButton } from './toolbar-icon-button';
export { default as __experimentalToolbarItem } from './toolbar-item';
export { default as Tooltip } from './tooltip';
export { default as TreeSelect } from './tree-select';
export { default as VisuallyHidden } from './visually-hidden';
Expand Down
2 changes: 2 additions & 0 deletions packages/components/src/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ export { default as Dropdown } from './dropdown';
export { default as Toolbar } from './toolbar';
export { default as ToolbarButton } from './toolbar-button';
export { default as ToolbarGroup } from './toolbar-group';
export { default as __experimentalToolbarIconButton } from './toolbar-icon-button';
export { default as __experimentalToolbarItem } from './toolbar-item';
export { default as Icon } from './icon';
export { default as IconButton } from './icon-button';
export { default as Spinner } from './spinner';
Expand Down
26 changes: 0 additions & 26 deletions packages/components/src/toolbar-button/accessible-toolbar-button-container.js
View file
Open in desktop

This file was deleted.

6 changes: 0 additions & 6 deletions packages/components/src/toolbar-button/accessible-toolbar-button-container.native.js
View file
Open in desktop

This file was deleted.

75 changes: 32 additions & 43 deletions packages/components/src/toolbar-button/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,13 @@
*/
import classnames from 'classnames';

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

/**
* Internal dependencies
*/
import IconButton from '../icon-button';
import ToolbarContext from '../toolbar-context';
import AccessibleToolbarButtonContainer from './accessible-toolbar-button-container';
import ToolbarButtonContainer from './toolbar-button-container';
import Button from '../button';
import ToolbarItem from '../toolbar-item';

function ToolbarButton( {
containerClassName,
Expand All @@ -28,48 +23,42 @@ function ToolbarButton( {
isDisabled,
extraProps,
children,
...props
} ) {
// It'll contain state if `ToolbarButton` is being used within
// `<Toolbar __experimentalAccessibilityLabel="label" />`
const accessibleToolbarState = useContext( ToolbarContext );

const button = (
<IconButton
icon={ icon }
label={ title }
shortcut={ shortcut }
data-subscript={ subscript }
onClick={ ( event ) => {
event.stopPropagation();
if ( onClick ) {
onClick( event );
}
} }
className={ classnames(
'components-toolbar__control',
className,
{ 'is-active': isActive }
) }
aria-pressed={ isActive }
disabled={ isDisabled }
{ ...extraProps }
/>
);

if ( accessibleToolbarState ) {
if ( icon ) {
return (
<AccessibleToolbarButtonContainer className={ containerClassName }>
{ button }
</AccessibleToolbarButtonContainer>
<ToolbarButtonContainer className={ containerClassName }>
<IconButton
icon={ icon }
label={ title }
shortcut={ shortcut }
data-subscript={ subscript }
onClick={ ( event ) => {
event.stopPropagation();
if ( onClick ) {
onClick( event );
}
} }
className={ classnames( 'components-toolbar__control', className, {
'is-active': isActive,
} ) }
aria-pressed={ isActive }
disabled={ isDisabled }
{ ...extraProps }
/>
{ children }
</ToolbarButtonContainer>
);
}

// ToolbarButton is being used outside of the accessible Toolbar
return (
<ToolbarButtonContainer className={ containerClassName }>
{ button }
{ children }
</ToolbarButtonContainer>
<ToolbarItem
onClick={ onClick }
className={ classnames( 'components-toolbar-button', className ) }
{ ...props }
>
{ ( toolbarItemProps ) => <Button { ...toolbarItemProps }>{ children }</Button> }
</ToolbarItem>
);
}

Expand Down
8 changes: 2 additions & 6 deletions packages/components/src/toolbar-group/toolbar-group-collapsed.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,3 @@
/**
* External dependencies
*/
import { ToolbarItem } from 'reakit/Toolbar';

/**
* WordPress dependencies
*/
Expand All @@ -13,6 +8,7 @@ import { useContext } from '@wordpress/element';
*/
import DropdownMenu from '../dropdown-menu';
import ToolbarContext from '../toolbar-context';
import ToolbarItem from '../toolbar-item';

function ToolbarGroupCollapsed( {
controls = [],
Expand Down Expand Up @@ -40,7 +36,7 @@ function ToolbarGroupCollapsed( {
if ( accessibleToolbarState ) {
return (
// https://reakit.io/docs/composition/#render-props
<ToolbarItem { ...accessibleToolbarState }>
<ToolbarItem>
{ ( toolbarItemHTMLProps ) => renderDropdownMenu( toolbarItemHTMLProps ) }
</ToolbarItem>
);
Expand Down
29 changes: 29 additions & 0 deletions packages/components/src/toolbar-icon-button/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/**
* External dependencies
*/
import classnames from 'classnames';

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

/**
* Internal dependencies
*/
import ToolbarItem from '../toolbar-item';
import IconButton from '../icon-button';

function ToolbarIconButton( props, ref ) {
const className = classnames(
'components-toolbar-icon-button',
props.className
);
return (
<ToolbarItem { ...props } className={ className } ref={ ref }>
{ ( toolbarItemProps ) => <IconButton { ...toolbarItemProps } /> }
</ToolbarItem>
);
}

export default forwardRef( ToolbarIconButton );
36 changes: 36 additions & 0 deletions packages/components/src/toolbar-item/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/**
* External dependencies
*/
import { useToolbarItem } from 'reakit/Toolbar';

/**
* WordPress dependencies
*/
import { forwardRef, useContext } from '@wordpress/element';

/**
* Internal dependencies
*/
import ToolbarContext from '../toolbar-context';

function ToolbarItem( { children, ...props }, ref ) {
const accessibleToolbarState = useContext( ToolbarContext );
// https://reakit.io/docs/composition/#props-hooks
const itemProps = useToolbarItem( accessibleToolbarState, { ...props, ref } );

if ( typeof children !== 'function' ) {
// eslint-disable-next-line no-console
console.warn( '`ToolbarItem` is a generic headless component that accepts only function children props' );
return null;
}

if ( ! accessibleToolbarState ) {
// eslint-disable-next-line no-console
console.warn( '`ToolbarItem` should be rendered within `<Toolbar __experimentalAccessibilityLabel="label">`' );
return null;
}

return children( itemProps );
}

export default forwardRef( ToolbarItem );
16 changes: 16 additions & 0 deletions packages/components/src/toolbar-item/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/**
* WordPress dependencies
*/
import { forwardRef } from '@wordpress/element';

function ToolbarItem( { children, ...props }, ref ) {
if ( typeof children !== 'function' ) {
// eslint-disable-next-line no-console
console.warn( '`ToolbarItem` is a generic headless component that accepts only function children props' );
return null;
}

return children( { ...props, ref } );
}

export default forwardRef( ToolbarItem );
57 changes: 38 additions & 19 deletions packages/components/src/toolbar/stories/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,15 @@
* Internal dependencies
*/
import Toolbar from '../';
import { SVG, Path, ToolbarButton, ToolbarGroup } from '../../';
import {
SVG,
Path,
ToolbarButton,
ToolbarGroup,
__experimentalToolbarIconButton as ToolbarIconButton,
__experimentalToolbarItem as ToolbarItem,
DropdownMenu,
} from '../../';

export default { title: 'Components|Toolbar', component: Toolbar };

Expand All @@ -20,22 +28,30 @@ export const _default = () => {
// id is required for server side rendering
<Toolbar __experimentalAccessibilityLabel="Options" id="options-toolbar">
<ToolbarGroup>
<ToolbarButton icon="editor-paragraph" title="Paragraph" />
<ToolbarIconButton icon="editor-paragraph" label="Paragraph" />
</ToolbarGroup>
<ToolbarGroup>
Copy link
Member

Choose a reason for hiding this comment

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

Is there any difference between this and:

Screen Shot 2020-01-03 at 12 09 57

It looks like we have the collapsed group case covered :)

Copy link
Member Author

Choose a reason for hiding this comment

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

There are some cases in Gutenberg where we're using DropdownMenu directly instead <ToolbarGroup isCollapsed>. For example:

gutenberg/packages/block-editor/src/components/block-settings-menu/index.js

Lines 59 to 66 in 2c1e3be

<Toolbar>
<DropdownMenu
icon="ellipsis"
label={ __( 'More options' ) }
className="block-editor-block-settings-menu"
popoverProps={ POPOVER_PROPS }
>
{ ( { onClose } ) => (

I'm not sure how easy is it to refactor those edge cases to use <ToolbarGroup isCollapsed>, so I just wanted to make sure ToolbarItem can be used.

Copy link
Member

Choose a reason for hiding this comment

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

Cool, it all makes sense. I think I mentioned it already, I just wanted to ensure you are aware of the current state of the art :)

<ToolbarItem>
{ ( toggleProps ) => (
<DropdownMenu
hasArrowIndicator
icon="editor-alignleft"
label="Change text alignment"
controls={ [
{ icon: 'editor-alignleft', title: 'Align left', isActive: true },
{ icon: 'editor-aligncenter', title: 'Align center' },
{ icon: 'editor-alignright', title: 'Align right' },
] }
toggleProps={ toggleProps }
/>
) }
</ToolbarItem>
</ToolbarGroup>
<ToolbarGroup
icon="editor-alignleft"
label="Change text alignment"
isCollapsed
controls={ [
{ icon: 'editor-alignleft', title: 'Align left', isActive: true },
{ icon: 'editor-aligncenter', title: 'Align center' },
{ icon: 'editor-alignright', title: 'Align right' },
] }
/>
<ToolbarGroup>
<ToolbarButton icon="editor-bold" title="Bold" />
<ToolbarButton icon="editor-italic" title="Italic" />
<ToolbarButton icon="admin-links" title="Link" />
<ToolbarButton>Text</ToolbarButton>
<ToolbarIconButton icon="editor-bold" label="Bold" />
<ToolbarIconButton icon="editor-italic" label="Italic" />
<ToolbarIconButton icon="admin-links" label="Link" />
<ToolbarGroup
isCollapsed
icon={ false }
Expand Down Expand Up @@ -65,10 +81,13 @@ export const _default = () => {
export const withoutGroup = () => {
return (
// id is required for server side rendering
<Toolbar __experimentalAccessibilityLabel="Options" id="options-toolbar-without-group">
<ToolbarButton icon="editor-bold" title="Bold" />
<ToolbarButton icon="editor-italic" title="Italic" />
<ToolbarButton icon="admin-links" title="Link" />
<Toolbar
__experimentalAccessibilityLabel="Options"
id="options-toolbar-without-group"
>
<ToolbarIconButton icon="editor-bold" label="Bold" />
<ToolbarIconButton icon="editor-italic" label="Italic" />
<ToolbarIconButton icon="admin-links" label="Link" />
</Toolbar>
);
};
Expand Down
6 changes: 3 additions & 3 deletions packages/components/src/toolbar/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,15 @@ import { mount } from 'enzyme';
* Internal dependencies
*/
import Toolbar from '../';
import ToolbarButton from '../../toolbar-button';
import ToolbarIconButton from '../../toolbar-icon-button';

describe( 'Toolbar', () => {
describe( 'basic rendering', () => {
it( 'should render a toolbar with toolbar buttons', () => {
const wrapper = mount(
<Toolbar __experimentalAccessibilityLabel="blocks">
<ToolbarButton title="control1" />
<ToolbarButton title="control2" />
<ToolbarIconButton label="control1" />
<ToolbarIconButton label="control2" />
</Toolbar>
);
const control1 = wrapper.find( 'button[aria-label="control1"]' );
Expand Down
Loading