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

Switching pattern categories inserter to Tabs component with arrow key navigation #60257

Merged
merged 24 commits into from
Apr 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
32f21e7
WIP for switching patterns to tab navigation
jeryj Mar 27, 2024
d9d8fd3
remove unused code
jeryj Mar 27, 2024
7b4fbf1
Let tabs component handle focus
jeryj Mar 28, 2024
4e83e4b
Pattern tab styles
jeryj Mar 28, 2024
12f9f24
Fix overflow scrolling on tablist
jeryj Mar 28, 2024
7acdeb9
Select first tab on patterns open
jeryj Mar 28, 2024
e8f8d9c
Add placeholder to offset zoom-out mode space for tabpanel
jeryj Mar 28, 2024
95951d2
Fix overflow on zoom out mode pattern inserter
jeryj Mar 28, 2024
96ebe8a
Refactor making space for side panel by using :after
jeryj Mar 29, 2024
74cd192
Fix performance test
jeryj Mar 29, 2024
1ec4685
Fix useZoomOut mode hook to only run when showing and hiding the patt…
jeryj Apr 2, 2024
494e72d
More tweaks to zoom out mode hook to simplify
jeryj Apr 2, 2024
a405a75
Use tabs in controlled mode so there is no initial category set
jeryj Apr 3, 2024
c2d2ecb
Allow tab to be active but not selected
jeryj Apr 8, 2024
00c5c20
Use selectedOnMount prop
jeryj Apr 8, 2024
1a44230
Revert "Allow tab to be active but not selected"
jeryj Apr 11, 2024
b7c495e
Revert "Use selectedOnMount prop"
jeryj Apr 11, 2024
91fffab
Remove artifacts from local e2e performance test
jeryj Apr 16, 2024
c15acb2
update tests to use the new tab role instead of button role
jeryj Apr 16, 2024
9918882
Temporary workaround for performance test
jeryj Apr 16, 2024
18d6355
Move some code back to its original location to make the review easier
jeryj Apr 18, 2024
2119306
Add missing __unstableSetEditorMode to useZoomOut useEffect dependency
jeryj Apr 18, 2024
bf2f485
Update packages/block-editor/src/hooks/use-zoom-out.js
jeryj Apr 18, 2024
a873e04
Fix use-zoom-out hooks dependencies
jeryj Apr 18, 2024
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
4 changes: 4 additions & 0 deletions packages/block-editor/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -1032,6 +1032,10 @@ _Returns_

A hook used to set the editor mode to zoomed out mode, invoking the hook sets the mode.

_Parameters_

- _zoomOut_ `boolean`: If we should enter into zoomOut mode or not

Comment on lines +1035 to +1038
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you help me understand why this change is part of this PR? What required the addition of this new param?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously the useZoomOut hook toggled between zoom in/out mode each time it ran. Passing a param of true/false allows us to intentionally choose if we want to use zoom out mode or not.

This param wasn't necessary in the previous set-up because the pattern category panel would unmount (turning zoom out mode off) then re mount a new category (turning zoom out mode back on). There's more context in this comment.

### Warning

_Related_
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,10 @@ import { useState } from '@wordpress/element';
import { __, isRTL } from '@wordpress/i18n';
import { useViewportMatch } from '@wordpress/compose';
import {
__experimentalItemGroup as ItemGroup,
__experimentalItem as Item,
__experimentalHStack as HStack,
FlexBlock,
Button,
privateApis as componentsPrivateApis,
} from '@wordpress/components';
import { Icon, chevronRight, chevronLeft } from '@wordpress/icons';

Expand All @@ -20,41 +19,48 @@ import PatternsExplorerModal from '../block-patterns-explorer';
import MobileTabNavigation from '../mobile-tab-navigation';
import { PatternCategoryPreviews } from './pattern-category-previews';
import { usePatternCategories } from './use-pattern-categories';
import { unlock } from '../../../lock-unlock';

const { Tabs } = unlock( componentsPrivateApis );

function BlockPatternsTab( {
onSelectCategory,
selectedCategory,
onInsert,
rootClientId,
children,
} ) {
const [ showPatternsExplorer, setShowPatternsExplorer ] = useState( false );

const categories = usePatternCategories( rootClientId );

const initialCategory = selectedCategory || categories[ 0 ];
const isMobile = useViewportMatch( 'medium', '<' );

return (
<>
{ ! isMobile && (
<div className="block-editor-inserter__block-patterns-tabs-container">
<nav
aria-label={ __( 'Block pattern categories' ) }
className="block-editor-inserter__block-patterns-tabs"
<Tabs
selectOnMove={ false }
selectedTabId={
selectedCategory ? selectedCategory.name : null
}
orientation={ 'vertical' }
onSelect={ ( categoryId ) => {
// Pass the full category object
onSelectCategory(
categories.find(
( category ) => category.name === categoryId
)
);
} }
>
<ItemGroup role="list">
<Tabs.TabList className="block-editor-inserter__block-patterns-tablist">
{ categories.map( ( category ) => (
<Item
role="listitem"
<Tabs.Tab
key={ category.name }
onClick={ () =>
onSelectCategory( category )
}
className={
category === selectedCategory
? 'block-editor-inserter__patterns-category block-editor-inserter__patterns-selected-category'
: 'block-editor-inserter__patterns-category'
}
tabId={ category.name }
className="block-editor-inserter__patterns-tab"
aria-label={ category.label }
aria-current={
category === selectedCategory
Expand All @@ -74,39 +80,47 @@ function BlockPatternsTab( {
}
/>
</HStack>
</Item>
</Tabs.Tab>
) ) }
<div role="listitem">
<Button
className="block-editor-inserter__patterns-explore-button"
onClick={ () =>
setShowPatternsExplorer( true )
}
variant="secondary"
>
{ __( 'Explore all patterns' ) }
</Button>
</div>
</ItemGroup>
</nav>
</Tabs.TabList>
{ categories.map( ( category ) => (
<Tabs.TabPanel
key={ category.name }
tabId={ category.name }
focusable={ false }
className="block-editor-inserter__patterns-category-panel"
>
{ children }
</Tabs.TabPanel>
) ) }
</Tabs>
<Button
className="block-editor-inserter__patterns-explore-button"
onClick={ () => setShowPatternsExplorer( true ) }
variant="secondary"
>
{ __( 'Explore all patterns' ) }
</Button>
</div>
) }
{ isMobile && (
<MobileTabNavigation categories={ categories }>
{ ( category ) => (
<PatternCategoryPreviews
key={ category.name }
onInsert={ onInsert }
rootClientId={ rootClientId }
category={ category }
showTitlesAsTooltip={ false }
/>
<div className="block-editor-inserter__patterns-category-panel">
<PatternCategoryPreviews
key={ category.name }
onInsert={ onInsert }
rootClientId={ rootClientId }
category={ category }
showTitlesAsTooltip={ false }
/>
</div>
) }
</MobileTabNavigation>
) }
{ showPatternsExplorer && (
<PatternsExplorerModal
initialCategory={ initialCategory }
initialCategory={ selectedCategory || categories[ 0 ] }
Copy link
Contributor

Choose a reason for hiding this comment

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

Are categories guaranteed to exist / be an array?

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 believe so. I didn't change how categories is used. I matched what trunk did.

patternCategories={ categories }
onModalClose={ () => setShowPatternsExplorer( false ) }
rootClientId={ rootClientId }
Expand Down
Original file line number Diff line number Diff line change
@@ -1,16 +1,8 @@
/**
* WordPress dependencies
*/
import { useRef, useEffect } from '@wordpress/element';

import { focus } from '@wordpress/dom';

/**
* Internal dependencies
*/

import { PatternCategoryPreviews } from './pattern-category-previews';
import { useZoomOut } from '../../../hooks/use-zoom-out';

export function PatternCategoryPreviewPanel( {
rootClientId,
Expand All @@ -20,34 +12,15 @@ export function PatternCategoryPreviewPanel( {
showTitlesAsTooltip,
patternFilter,
} ) {
const container = useRef();

useEffect( () => {
const timeout = setTimeout( () => {
const [ firstTabbable ] = focus.tabbable.find( container.current );
firstTabbable?.focus();
} );
return () => clearTimeout( timeout );
}, [ category ] );

// Move to zoom out mode when this component is mounted
// and back to the previous mode when unmounted.
useZoomOut();

return (
Comment on lines -23 to -36
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain why it was ok to remove all this code? It seems it was handling zoom out and autofocus.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This did handle autofocus and zoom out. I'd like to remove autofocus, as none of the other tab components move focus into the panel, nor is it mentioned on the tabs pattern ARIA guide. I played around with it a bit on the keyboard and quite liked not moving autofocus for being able to activate and see the patterns in a category without committing to the category. It makes browsing categories a lot easier IMO.

The zoom out mode hook had to be moved up a level to the menu.js.

<div
ref={ container }
className="block-editor-inserter__patterns-category-dialog"
>
<PatternCategoryPreviews
key={ category.name }
rootClientId={ rootClientId }
onInsert={ onInsert }
onHover={ onHover }
category={ category }
showTitlesAsTooltip={ showTitlesAsTooltip }
patternFilter={ patternFilter }
/>
</div>
<PatternCategoryPreviews
key={ category.name }
rootClientId={ rootClientId }
onInsert={ onInsert }
onHover={ onHover }
category={ category }
showTitlesAsTooltip={ showTitlesAsTooltip }
patternFilter={ patternFilter }
/>
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ export function PatternCategoryPreviews( {
);

return (
<div className="block-editor-inserter__patterns-category-panel">
<>
<VStack
spacing={ 2 }
className="block-editor-inserter__patterns-category-panel-header"
Expand Down Expand Up @@ -174,6 +174,6 @@ export function PatternCategoryPreviews( {
pagingProps={ pagingProps }
/>
) }
</div>
</>
);
}
43 changes: 27 additions & 16 deletions packages/block-editor/src/components/inserter/menu.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import InserterSearchResults from './search-results';
import useInsertionPoint from './hooks/use-insertion-point';
import InserterTabs from './tabs';
import { store as blockEditorStore } from '../../store';
import { useZoomOut } from '../../hooks/use-zoom-out';

const NOOP = () => {};
function InserterMenu(
Expand Down Expand Up @@ -125,6 +126,11 @@ function InserterMenu(
[ setSelectedPatternCategory, __experimentalOnPatternCategorySelection ]
);

const showPatternPanel =
selectedTab === 'patterns' &&
! delayedFilterValue &&
selectedPatternCategory;

const blocksTab = useMemo(
() => (
<>
Expand Down Expand Up @@ -162,13 +168,25 @@ function InserterMenu(
onInsert={ onInsertPattern }
onSelectCategory={ onClickPatternCategory }
selectedCategory={ selectedPatternCategory }
/>
>
{ showPatternPanel && (
<PatternCategoryPreviewPanel
rootClientId={ destinationRootClientId }
onInsert={ onInsertPattern }
onHover={ onHoverPattern }
category={ selectedPatternCategory }
patternFilter={ patternFilter }
showTitlesAsTooltip
/>
) }
</BlockPatternsTab>
),
[
destinationRootClientId,
onInsertPattern,
onClickPatternCategory,
selectedPatternCategory,
showPatternPanel,
]
);

Expand Down Expand Up @@ -205,16 +223,15 @@ function InserterMenu(
},
} ) );

const showPatternPanel =
selectedTab === 'patterns' &&
! delayedFilterValue &&
selectedPatternCategory;
const showAsTabs = ! delayedFilterValue && ( showPatterns || showMedia );
const showMediaPanel =
selectedTab === 'media' &&
! delayedFilterValue &&
selectedMediaCategory;

// When the pattern panel is showing, we want to use zoom out mode
useZoomOut( showPatternPanel );
Copy link
Contributor

Choose a reason for hiding this comment

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

Why have we moved this here, rather than keeping it inside PatternCategoryPreviewPanel?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before, the PatternCategoryPreviewPanel was mounted and then the patterns switched out inside of it. Now, each panel is its own component (via the tabs component). So, if we left it within PatternCategoryPreviewPanel it would rerun the hook each time you switched categories, toggling between zoom out and edit. By moving it up a level we can preserve the right mode.


const handleSetSelectedTab = ( value ) => {
// If no longer on patterns tab remove the category setting.
if ( value !== 'patterns' ) {
Expand All @@ -224,7 +241,11 @@ function InserterMenu(
};

return (
<div className="block-editor-inserter__menu">
<div
className={ classnames( 'block-editor-inserter__menu', {
'show-panel': showPatternPanel,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why this class name is show-panel and the toggle bool is showPatternPanel - shouldn't the class be about pattern panel as well?

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 left it generic, as the panel is a fly out of the menu for anything to get put into. For example, the media tab should get changed to this pattern as well. We could also add a secondary class that says it's a pattern panel if needed.

} ) }
>
<div
className={ classnames( 'block-editor-inserter__main-area', {
'show-as-tabs': showAsTabs,
Expand Down Expand Up @@ -292,16 +313,6 @@ function InserterMenu(
<InserterPreviewPanel item={ hoveredItem } />
</Popover>
) }
{ showPatternPanel && (
<PatternCategoryPreviewPanel
rootClientId={ destinationRootClientId }
onInsert={ onInsertPattern }
onHover={ onHoverPattern }
category={ selectedPatternCategory }
patternFilter={ patternFilter }
showTitlesAsTooltip
/>
) }
</div>
);
}
Expand Down
Loading
Loading