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

Add TabPanel to document overview replacing fake tabs #50199

Merged
merged 17 commits into from
May 30, 2023
Merged

Add TabPanel to document overview replacing fake tabs #50199

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
7ea8b3d
Add TabPanel to document overview sidebar.
alexstine Apr 29, 2023
d01d543
TabPanel: Changed to support ref forwarding. Added prop for attaching…
alexstine Apr 29, 2023
3e24a0c
Merge branch 'trunk' of github.com:wordpress/gutenberg into add/tabpa…
alexstine May 19, 2023
f18882a
Reviewer feedback addressed.
alexstine May 19, 2023
a2d4dd6
Fix E2E tests.
alexstine May 19, 2023
3de13aa
Update styles to more closely reflect the look of the tabs in trunk
andrewserong May 22, 2023
202869c
Restore scrollbar behaviour
andrewserong May 23, 2023
9c6e0c1
Add changelog entry for components package
andrewserong May 23, 2023
0843cee
Merge branch 'trunk' of github.com:wordpress/gutenberg into add/tabpa…
alexstine May 23, 2023
1487a0b
Better comments.
alexstine May 23, 2023
4542b3f
Fix conflict.
alexstine May 26, 2023
227aeda
Fix E2E.
alexstine May 26, 2023
cf8c4ba
Add back useState for cleaner code.
alexstine May 26, 2023
362e0e9
Fix list view prop. Changelog update.
alexstine May 27, 2023
9bc9fbb
Fix tab panel focus. Still need to use focus.tabbable.find().
alexstine May 27, 2023
5b1543d
Fix conflict.
alexstine May 27, 2023
f984f79
Fix changelog format.
alexstine May 27, 2023
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
32 changes: 20 additions & 12 deletions packages/components/src/tab-panel/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,13 @@
* External dependencies
*/
import classnames from 'classnames';
import type { ForwardedRef } from 'react';

/**
* WordPress dependencies
*/
import {
forwardRef,
useState,
useEffect,
useLayoutEffect,
Expand Down Expand Up @@ -76,16 +78,20 @@ const TabButton = ( {
* );
* ```
*/
export function TabPanel( {
className,
children,
tabs,
selectOnMove = true,
initialTabName,
orientation = 'horizontal',
activeClass = 'is-active',
onSelect,
}: WordPressComponentProps< TabPanelProps, 'div', false > ) {
const UnforwardedTabPanel = (
{
className,
children,
tabs,
selectOnMove = true,
initialTabName,
orientation = 'horizontal',
activeClass = 'is-active',
onSelect,
itemRef,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure why TS is not forcing me to type this. also not sure how to type it. I would expect this value to take either a single ref or merged refs from useMergedRefs hook.

}: WordPressComponentProps< TabPanelProps, 'div', false >,
ref: ForwardedRef< any >
) => {
const instanceId = useInstanceId( TabPanel, 'tab-panel' );
const [ selected, setSelected ] = useState< string >();

Expand Down Expand Up @@ -151,7 +157,7 @@ export function TabPanel( {
}, [ tabs, selectedTab?.disabled, handleTabSelection ] );

return (
<div className={ className }>
<div className={ className } ref={ ref }>
<NavigableMenu
role="tablist"
orientation={ orientation }
Expand Down Expand Up @@ -190,12 +196,14 @@ export function TabPanel( {
role="tabpanel"
id={ `${ selectedId }-view` }
className="components-tab-panel__tab-content"
ref={ itemRef || undefined }
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 should never be required, only complementary. Useful for managing focus in rendered children.

Copy link
Member

Choose a reason for hiding this comment

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

As context, the components crew is working on a new version of the TabPanel component that would support these kinds of features in a more ergonomic API. So ideally, I'd like to make this fix to the Document Overview without adding new APIs to the current TabPanel component (i.e. the itemRef prop).

Would it be possible to handle the item ref concerns directly in the consumer (.edit-post-editor__list-view-container) without sending it through the TabPanel component?

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 do not think so, not for this case specifically. I need a way to always make sure I know where my ref is attached for focus reasons. I hate to do it but trying to guess focus is not going to work for this. I could try to refactor it but it will likely not be pretty.

Copy link
Member

Choose a reason for hiding this comment

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

Just to be clear, I mean something like this. It seems to work but I may not be picking up on all the focus management concerns — are there any requirements that this doesn't cover?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me give it a test later and see. Thanks for the quick commit @mirka. If it works, it will indeed be simpler than what I was trying to do. I kept getting a rules of hooks violation but I didn't think about using useMergeRefs() earlier.

>
{ children( selectedTab ) }
</div>
) }
</div>
);
}
};

export const TabPanel = forwardRef( UnforwardedTabPanel );
export default TabPanel;
124 changes: 52 additions & 72 deletions packages/edit-post/src/components/secondary-sidebar/list-view-sidebar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,16 @@
/**
* External dependencies
*/
import classnames from 'classnames';

/**
* WordPress dependencies
*/
import { __experimentalListView as ListView } from '@wordpress/block-editor';
import { Button } from '@wordpress/components';
import { Button, TabPanel } from '@wordpress/components';
import {
useFocusOnMount,
useFocusReturn,
useMergeRefs,
} from '@wordpress/compose';
import { useDispatch } from '@wordpress/data';
import { focus } from '@wordpress/dom';
import { useRef, useState } from '@wordpress/element';
import { useRef } from '@wordpress/element';
import { __ } from '@wordpress/i18n';
import { closeSmall } from '@wordpress/icons';
import { useShortcut } from '@wordpress/keyboard-shortcuts';
Expand All @@ -41,41 +36,35 @@ export default function ListViewSidebar() {
}
}

const [ tab, setTab ] = useState( 'list-view' );

// This ref refers to the sidebar as a whole.
const sidebarRef = useRef();
// This ref refers to the list view tab button.
const listViewTabRef = useRef();
// This ref refers to the outline tab button.
const outlineTabRef = useRef();
// This ref refers to the tab panel.
const tabPanelRef = useRef();
// This ref refers to the list view application area.
const listViewRef = useRef();

/*
* Callback function to handle list view or outline focus.
*
* @param {string} currentTab The current tab. Either list view or outline.
*
* @return void
*/
function handleSidebarFocus( currentTab ) {
// List view tab is selected.
if ( currentTab === 'list-view' ) {
// Either focus the list view or the list view tab button. Must have a fallback because the list view does not render when there are no blocks.
const listViewApplicationFocus = focus.tabbable.find(
listViewRef.current
)[ 0 ];
const listViewFocusArea = sidebarRef.current.contains(
listViewApplicationFocus
)
? listViewApplicationFocus
: listViewTabRef.current;
listViewFocusArea.focus();
// Outline tab is selected.
} else {
outlineTabRef.current.focus();
function handleSidebarFocus() {
alexstine marked this conversation as resolved.
Show resolved Hide resolved
// Either focus the list view or the tab panel. Must have a fallback because the list view does not render when there are no blocks.
const listViewApplicationFocus = focus.tabbable.find(
listViewRef.current
)[ 0 ];
const listViewFocusArea = sidebarRef.current.contains(
listViewApplicationFocus
)
? listViewApplicationFocus
: focus.tabbable.find( tabPanelRef.current )[ 0 ];
// Do not focus when focus is already there.
if (
sidebarRef.current.ownerDocument.activeElement === listViewFocusArea
) {
return;
}
listViewFocusArea.focus();
}

// This only fires when the sidebar is open because of the conditional rendering. It is the same shortcut to open but that is defined as a global shortcut and only fires when the sidebar is closed.
Expand All @@ -89,10 +78,21 @@ export default function ListViewSidebar() {
setIsListViewOpened( false );
// If the list view or outline does not have focus, focus should be moved to it.
} else {
handleSidebarFocus( tab );
handleSidebarFocus();
}
} );

function renderTabContent( tabName ) {
if ( tabName === 'list-view' ) {
return (
<div className="edit-post-editor__list-view-panel-content">
<ListView />
</div>
);
}
return <ListViewOutline />;
}

return (
// eslint-disable-next-line jsx-a11y/no-static-element-interactions
<div
Expand All @@ -109,54 +109,34 @@ export default function ListViewSidebar() {
label={ __( 'Close' ) }
onClick={ () => setIsListViewOpened( false ) }
/>
<ul>
<li>
<Button
ref={ listViewTabRef }
onClick={ () => {
setTab( 'list-view' );
} }
className={ classnames(
'edit-post-sidebar__panel-tab',
{ 'is-active': tab === 'list-view' }
) }
aria-current={ tab === 'list-view' }
>
{ __( 'List View' ) }
</Button>
</li>
<li>
<Button
ref={ outlineTabRef }
onClick={ () => {
setTab( 'outline' );
} }
className={ classnames(
'edit-post-sidebar__panel-tab',
{ 'is-active': tab === 'outline' }
) }
aria-current={ tab === 'outline' }
>
{ __( 'Outline' ) }
</Button>
</li>
</ul>
</div>
<div
ref={ useMergeRefs( [
<TabPanel
ref={ tabPanelRef }
selectOnMove={ false }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Manual activation due to focus handling with the list view.

itemRef={ useMergeRefs( [
contentFocusReturnRef,
focusOnMountRef,
listViewRef,
] ) }
className="edit-post-editor__list-view-container"
tabs={ [
{
name: 'list-view',
title: 'List View',
className: 'edit-post-sidebar__panel-tab',
},
{
name: 'outline',
title: 'Outline',
className: 'edit-post-sidebar__panel-tab',
},
] }
>
{ tab === 'list-view' && (
<div className="edit-post-editor__list-view-panel-content">
<ListView />
{ ( tab ) => (
<div className="edit-post-editor__list-view-container">
{ renderTabContent( tab.name ) }
</div>
) }
{ tab === 'outline' && <ListViewOutline /> }
</div>
</TabPanel>
</div>
);
}