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

Jump to bottom

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 all 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
1 change: 1 addition & 0 deletions packages/components/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ 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 __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
1 change: 1 addition & 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,7 @@ 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 __experimentalToolbarItem } from './toolbar-item';
export { default as Icon } from './icon';
export { default as IconButton } from './button/deprecated';
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.

74 changes: 33 additions & 41 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 @@ -12,63 +12,55 @@ import { useContext } from '@wordpress/element';
* Internal dependencies
*/
import Button from '../button';
import ToolbarItem from '../toolbar-item';
import ToolbarContext from '../toolbar-context';
import AccessibleToolbarButtonContainer from './accessible-toolbar-button-container';
import ToolbarButtonContainer from './toolbar-button-container';

function ToolbarButton( {
containerClassName,
icon,
title,
shortcut,
subscript,
onClick,
className,
isActive,
isDisabled,
extraProps,
children,
...props
Comment on lines 19 to +24
Copy link
Member Author

Choose a reason for hiding this comment

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

Removed most of the props here so we can spread { ...props } directly into Button. This way, ToolbarButton (when used within <Toolbar __experimentalAccessibilityLabel="label">) and Button have identical API.

} ) {
// It'll contain state if `ToolbarButton` is being used within
// `<Toolbar __experimentalAccessibilityLabel="label" />`
const accessibleToolbarState = useContext( ToolbarContext );

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

if ( accessibleToolbarState ) {
if ( ! accessibleToolbarState ) {
// This should be deprecated when <Toolbar __experimentalAccessibilityLabel="label">
// becomes stable.
return (
<AccessibleToolbarButtonContainer className={ containerClassName }>
{ button }
</AccessibleToolbarButtonContainer>
<ToolbarButtonContainer className={ containerClassName }>
<Button
icon={ props.icon }
label={ props.title }
Copy link
Member

Choose a reason for hiding this comment

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

Does it mean that title should be deprecated in favor of label?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd say that these props should be deprecated:

  • title (in favor of label)
  • isActive (in favor of isPressed)
  • isDisabled (in favor of disabled)

Still not sure what to do with isDisabled though.

This PR only changes the behavior of ToolbarButton when it's used inside <Toolbar __experimentalAccessibilityLabel>, so if we decide to deprecate those props, I guess it should be in another PR.

Copy link
Member

Choose a reason for hiding this comment

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

I like the idea, let's go this way in the follow-up PRs.

shortcut={ props.shortcut }
data-subscript={ props.subscript }
Copy link
Member

Choose a reason for hiding this comment

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

I will investigate in a follow-up whether we can get rid of subscript as I think we no longer use it in code and it probably was never meant to exist as public API.

Copy link
Member Author

Choose a reason for hiding this comment

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

I kept it there so as to avoid breaking anything, but it's not being used in the new implementation (using ToolbarItem) anyway.

onClick={ ( event ) => {
event.stopPropagation();
if ( props.onClick ) {
props.onClick( event );
}
} }
className={ classnames( 'components-toolbar__control', className ) }
isPressed={ props.isActive }
Copy link
Member

Choose a reason for hiding this comment

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

A similar question, should we align API and deprecated isActive in favor of isPressed?

Copy link
Member

Choose a reason for hiding this comment

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

In stories, I still can see isActive:
Screen Shot 2020-01-03 at 12 03 10

Copy link
Member Author

Choose a reason for hiding this comment

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

That's the current API. This PR only changes ToolbarButton when used within <Toolbar __experimentalAccessibilityLabel>.

Copy link
Member

Choose a reason for hiding this comment

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

Right, we will need to align it at some point with the Button component as discussed.

disabled={ props.isDisabled }
Copy link
Member

Choose a reason for hiding this comment

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

I think this was discussed in other PRs, we should make a final call on whether we use isDisabled in all places instead of disabled to potentially break the direct association with the HTML attribute.

/cc @youknowriad @aduth

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it makes sense to adopt isDisabled. Specially if #19337 is merged as it'll not be the same as the HTML disabled prop.

{ ...extraProps }
/>
{ children }
</ToolbarButtonContainer>
);
}

// ToolbarButton is being used outside of the accessible Toolbar
// ToobarItem will pass all props to the render prop child, which will pass
// all props to Button. This means that ToolbarButton has the same API as
// Button.
return (
<ToolbarButtonContainer className={ containerClassName }>
{ button }
{ children }
</ToolbarButtonContainer>
<ToolbarItem
className={ classnames( 'components-toolbar-button', className ) }
{ ...props }
>
{ ( toolbarItemProps ) => <Button { ...toolbarItemProps }>{ children }</Button> }
</ToolbarItem>
);
}

Expand Down
8 changes: 3 additions & 5 deletions packages/components/src/toolbar-group/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,8 @@ function ToolbarGroup( {
children,
className,
isCollapsed,
icon,
title,
...otherProps
...props
} ) {
// It'll contain state if `ToolbarGroup` is being used within
// `<Toolbar accessibilityLabel="label" />`
Expand All @@ -81,18 +80,17 @@ function ToolbarGroup( {
if ( isCollapsed ) {
return (
<ToolbarGroupCollapsed
icon={ icon }
label={ title }
controls={ controlSets }
className={ finalClassName }
children={ children }
{ ...otherProps }
{ ...props }
/>
);
}

return (
<ToolbarGroupContainer className={ finalClassName } { ...otherProps }>
<ToolbarGroupContainer className={ finalClassName } { ...props }>
{ flatMap( controlSets, ( controlSet, indexOfSet ) =>
controlSet.map( ( control, indexOfControl ) => (
<ToolbarButton
Expand Down
24 changes: 3 additions & 21 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,37 +8,24 @@ import { useContext } from '@wordpress/element';
*/
import DropdownMenu from '../dropdown-menu';
import ToolbarContext from '../toolbar-context';
import ToolbarItem from '../toolbar-item';

function ToolbarGroupCollapsed( {
controls = [],
className,
icon,
label,
...props
} ) {
function ToolbarGroupCollapsed( { controls = [], ...props } ) {
// It'll contain state if `ToolbarGroup` is being used within
// `<Toolbar __experimentalAccessibilityLabel="label" />`
const accessibleToolbarState = useContext( ToolbarContext );

const renderDropdownMenu = ( toggleProps ) => (
<DropdownMenu
hasArrowIndicator
icon={ icon }
label={ label }
controls={ controls }
className={ className }
toggleProps={ toggleProps }
{ ...props }
/>
);

if ( accessibleToolbarState ) {
return (
// https://reakit.io/docs/composition/#render-props
<ToolbarItem { ...accessibleToolbarState }>
{ ( toolbarItemHTMLProps ) => renderDropdownMenu( toolbarItemHTMLProps ) }
</ToolbarItem>
);
return <ToolbarItem>{ renderDropdownMenu }</ToolbarItem>;
}

return renderDropdownMenu();
Expand Down
12 changes: 2 additions & 10 deletions packages/components/src/toolbar-group/toolbar-group-collapsed.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,8 @@
*/
import DropdownMenu from '../dropdown-menu';

function ToolbarGroupCollapsed( { controls = [], className, icon, label } ) {
return (
<DropdownMenu
hasArrowIndicator
icon={ icon }
label={ label }
controls={ controls }
className={ className }
/>
);
function ToolbarGroupCollapsed( { controls = [], ...props } ) {
return <DropdownMenu hasArrowIndicator controls={ controls } { ...props } />;
}

export default ToolbarGroupCollapsed;
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' );
diegohaz marked this conversation as resolved.
Show resolved Hide resolved
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 );
56 changes: 37 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,14 @@
* Internal dependencies
*/
import Toolbar from '../';
import { SVG, Path, ToolbarButton, ToolbarGroup } from '../../';
import {
SVG,
Path,
ToolbarButton,
ToolbarGroup,
__experimentalToolbarItem as ToolbarItem,
DropdownMenu,
} from '../../';

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

Expand All @@ -20,22 +27,30 @@ export const _default = () => {
// id is required for server side rendering
<Toolbar __experimentalAccessibilityLabel="Options" id="options-toolbar">
<ToolbarGroup>
<ToolbarButton icon="editor-paragraph" title="Paragraph" />
<ToolbarButton 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>
<ToolbarButton icon="editor-bold" label="Bold" isPressed />
<ToolbarButton icon="editor-italic" label="Italic" />
<ToolbarButton icon="admin-links" label="Link" />
<ToolbarGroup
isCollapsed
icon={ false }
Expand Down Expand Up @@ -65,10 +80,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"
>
<ToolbarButton icon="editor-bold" label="Bold" isPressed />
<ToolbarButton icon="editor-italic" label="Italic" />
<ToolbarButton icon="admin-links" label="Link" />
</Toolbar>
);
};
Expand Down
4 changes: 2 additions & 2 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 @@ -14,8 +14,8 @@ describe( 'Toolbar', () => {
it( 'should render a toolbar with toolbar buttons', () => {
const wrapper = mount(
<Toolbar __experimentalAccessibilityLabel="blocks">
<ToolbarButton title="control1" />
<ToolbarButton title="control2" />
<ToolbarButton label="control1" />
<ToolbarButton label="control2" />
Copy link
Member Author

Choose a reason for hiding this comment

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

When used within <Toolbar __experimentalAccessibilityLabel>, ToolbarButton passes all the props down to Button so they have the same API.

Copy link
Member

Choose a reason for hiding this comment

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

Nice 👍

</Toolbar>
);
const control1 = wrapper.find( 'button[aria-label="control1"]' );
Expand Down
Loading