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

Use composite pattern to improve keyboard navigation on the inserter #23610

Merged
merged 5 commits into from
Jul 15, 2020
+47 −7
Merged

Use composite pattern to improve keyboard navigation on the inserter #23610

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
85a2a0c
Use composite pattern to improve keyboard navigation on the inserter
youknowriad Jul 1, 2020
ee899f9
Address a11y feedback
youknowriad Jul 2, 2020
c682202
Fix composite bug
youknowriad Jul 2, 2020
506de60
Use composite.unstable_sort and update html structure
diegohaz Jul 15, 2020
595010e
Fix end2end tests
youknowriad Jul 15, 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 package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions packages/block-editor/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@
"react-autosize-textarea": "^3.0.2",
"react-spring": "^8.0.19",
"react-transition-group": "^2.9.0",
"reakit": "^1.1.0",
"redux-multi": "^0.1.12",
"refx": "^3.0.0",
"rememo": "^3.0.0",
Expand Down
24 changes: 22 additions & 2 deletions packages/block-editor/src/components/block-types-list/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
/**
* External dependencies
*/
import { Composite, useCompositeState } from 'reakit';
Copy link
Member

Choose a reason for hiding this comment

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

It adds nearly 10kB to the module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I was wondering about that. the solution could be to make it part of the @wordpress/components API but then it becomes something we need to ensure BC for. Always a struggle to decide what's best in these situations.

Copy link
Member

@gziolo gziolo Jul 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Let's improve the experience first and see how the performance might get improved later then.

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'm hoping we can balance these kbits loss by refactoring some of our components to rely on reakit like NavigableToolbar/NavigableContainer...

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it's essentially what Composite does 👍


/**
* WordPress dependencies
*/
Expand All @@ -14,16 +19,30 @@ function BlockTypesList( {
onSelect,
onHover = () => {},
children,
label,
} ) {
const composite = useCompositeState();
const normalizedItems = includeVariationsInInserterItems( items );
const orderId = normalizedItems.reduce(
( acc, item ) => acc + '--' + item.id,
''
);

return (
/*
* Disable reason: The `list` ARIA role is redundant but
* Safari+VoiceOver won't announce the list otherwise.
*/
/* eslint-disable jsx-a11y/no-redundant-roles */
<ul role="list" className="block-editor-block-types-list">
<Composite
Copy link
Member

Choose a reason for hiding this comment

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

Do you think we should expose Composite or maybe create a Listbox component in @wordpress/components? We can live with 10kB extra for now, but I'm sure there is going to be more cases where Reakit can be useful :)

as="ul"
role="listbox"
{ ...composite }
className="block-editor-block-types-list"
aria-label={ label }
// This ensures the composite state refreshes when the list order changes.
key={ orderId }
>
{ normalizedItems.map( ( item ) => {
return (
<InserterListItem
Expand All @@ -40,11 +59,12 @@ function BlockTypesList( {
onBlur={ () => onHover( null ) }
isDisabled={ item.isDisabled }
title={ item.title }
composite={ composite }
youknowriad marked this conversation as resolved.
Show resolved Hide resolved
/>
);
} ) }
{ children }
</ul>
</Composite>
/* eslint-enable jsx-a11y/no-redundant-roles */
);
}
Expand Down
10 changes: 7 additions & 3 deletions packages/block-editor/src/components/inserter-list-item/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* External dependencies
*/
import classnames from 'classnames';
import { CompositeItem } from 'reakit';

/**
* WordPress dependencies
Expand All @@ -19,6 +20,7 @@ function InserterListItem( {
isDisabled,
title,
className,
composite,
...props
} ) {
const itemIconStyle = icon
Expand All @@ -29,8 +31,10 @@ function InserterListItem( {
: {};

return (
<li className="block-editor-block-types-list__list-item">
<Button
<li className="block-editor-block-types-list__list-item" role="option">
<CompositeItem
as={ Button }
{ ...composite }
className={ classnames(
'block-editor-block-types-list__item',
className
Expand All @@ -51,7 +55,7 @@ function InserterListItem( {
<span className="block-editor-block-types-list__item-title">
{ title }
</span>
</Button>
</CompositeItem>
</li>
);
}
Expand Down
5 changes: 5 additions & 0 deletions packages/block-editor/src/components/inserter/block-types-tab.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ export function BlockTypesTab( {
items={ filteredItems }
onSelect={ onSelectItem }
onHover={ onHover }
label={ __( 'Child Blocks' ) }
/>
</ChildBlocks>
) }
Expand All @@ -145,6 +146,7 @@ export function BlockTypesTab( {
items={ suggestedItems }
onSelect={ onSelectItem }
onHover={ onHover }
label={ _x( 'Most used', 'blocks' ) }
/>
</InserterPanel>
) }
Expand All @@ -165,6 +167,7 @@ export function BlockTypesTab( {
items={ categoryItems }
onSelect={ onSelectItem }
onHover={ onHover }
label={ category.title }
/>
</InserterPanel>
);
Expand All @@ -179,6 +182,7 @@ export function BlockTypesTab( {
items={ uncategorizedItems }
onSelect={ onSelectItem }
onHover={ onHover }
label={ __( 'Uncategorized' ) }
/>
</InserterPanel>
) }
Expand All @@ -200,6 +204,7 @@ export function BlockTypesTab( {
items={ collectionItems }
onSelect={ onSelectItem }
onHover={ onHover }
label={ collection.title }
/>
</InserterPanel>
);
Expand Down
1 change: 1 addition & 0 deletions packages/block-editor/src/components/inserter/quick-inserter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ function QuickInserterList( {
items={ shownBlockTypes }
onSelect={ onSelectBlockType }
onHover={ onHover }
label={ __( 'Blocks' ) }
/>
</InserterPanel>
) }
Expand Down
5 changes: 5 additions & 0 deletions packages/block-editor/src/components/inserter/reusable-blocks-tab.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,11 @@ function ReusableBlocksList( {
items={ filteredItems }
onSelect={ onSelectItem }
onHover={ onHover }
label={
filterValue
? __( 'Search Results' )
: __( 'Reusable blocks' )
}
/>
</InserterPanel>
);
Expand Down