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

TabPanel: implement Ariakit internally #52133

Merged
merged 30 commits into from
Aug 2, 2023
Merged
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
f20e989
Add Ariakit-powered version of TabPanel
chad1008 Jun 15, 2023
8b002ae
Copy `TabPanel` stories over to `Tabs`
chad1008 Jun 15, 2023
3f9b7e0
Add updated versions of `TabPanel` unit tests
chad1008 Jun 15, 2023
97a4edc
add "Manual Activation" story
chad1008 Jun 19, 2023
bbac641
switch to async/findby based unit testing
chad1008 Jun 28, 2023
3795477
update changelog
chad1008 Jun 29, 2023
5ede3dc
remove old radix dependency
chad1008 Jul 5, 2023
1bab116
rename monolithic component to `TabPanel` for consistency
chad1008 Jul 5, 2023
75f090c
add `useInstanceId` support
chad1008 Jul 6, 2023
ea1e21e
add className and id parity with existing component
chad1008 Jul 6, 2023
dac69b7
update setSelectedId helper
chad1008 Jul 6, 2023
e7b0aa2
apply Ariakit internals to existing TabPanel component
chad1008 Jul 6, 2023
6195305
update TabPanel unit tests to work with Ariakit internals
chad1008 Jul 6, 2023
68fcf7e
remove temporary separate/new component files
chad1008 Jul 12, 2023
b9e019c
Update packages/components/src/tab-panel/stories/index.tsx
chad1008 Jul 13, 2023
37c014b
update CHANGELOG entry
chad1008 Jul 13, 2023
c89aedd
restore package-lock.json
chad1008 Jul 13, 2023
582afa5
remove unnecessary `await`s
chad1008 Jul 13, 2023
c97cbee
move `extractTabName` out of component
chad1008 Jul 13, 2023
00d2eaa
restore original component lazy loading
chad1008 Jul 24, 2023
4b7d7bb
update CHANGELOG entry
chad1008 Jul 24, 2023
aa1bfc2
up editor preferences modal unit tests
chad1008 Jul 24, 2023
defdb27
merge trunk into add/ariakit-tabs
chad1008 Jul 25, 2023
9731da4
remove onSelect call count assertions in unit test
chad1008 Jul 26, 2023
9507f4f
update snapshot and add a component-level test to ensure proper tabin…
chad1008 Jul 26, 2023
d938bd3
merge trunk into add/ariakit-tabs
chad1008 Jul 26, 2023
60c70d5
merge trunk into add/ariakit-tabs
chad1008 Aug 2, 2023
6b98f0a
revert accidental change from `classnames` to `cx`
chad1008 Aug 2, 2023
1782f26
Update CHANGELOG entry
chad1008 Aug 2, 2023
4924d02
remove unneeded `key`
chad1008 Aug 2, 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
30 changes: 15 additions & 15 deletions .github/workflows/enforce-pr-labels.yml
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
name: Enforce labels on Pull Request
on:
pull_request_target:
types: [opened, labeled, unlabeled, synchronize]
pull_request_target:
types: [opened, labeled, unlabeled, synchronize]
jobs:
type-related-labels:
runs-on: ubuntu-latest
permissions:
pull-requests: write
steps:
- uses: mheap/github-action-required-labels@v5
with:
mode: exactly
count: 1
labels: "[Type] Accessibility (a11y), [Type] Automated Testing, [Type] Breaking Change, [Type] Bug, [Type] Build Tooling, [Type] Code Quality, [Type] Copy, [Type] Developer Documentation, [Type] Enhancement, [Type] Experimental, [Type] Feature, [Type] New API, [Type] Task, [Type] Performance, [Type] Project Management, [Type] Security, [Type] WP Core Ticket"
add_comment: true
message: "## ⚠️ Type of PR label error\n To merge this PR, it requires {{ errorString }} {{ count }} label indicating the type of PR. Other labels are optional and not being checked here. \n- **Type-related labels to choose from**: {{ provided }}.\n- **Labels found**: {{ applied }}."
exit_type: success
type-related-labels:
runs-on: ubuntu-latest
permissions:
pull-requests: write
steps:
- uses: mheap/github-action-required-labels@v5
with:
mode: exactly
count: 1
labels: '[Type] Accessibility (a11y), [Type] Automated Testing, [Type] Breaking Change, [Type] Bug, [Type] Build Tooling, [Type] Code Quality, [Type] Copy, [Type] Developer Documentation, [Type] Enhancement, [Type] Experimental, [Type] Feature, [Type] New API, [Type] Task, [Type] Performance, [Type] Project Management, [Type] Security, [Type] WP Core Ticket'
add_comment: true
message: "## ⚠️ Type of PR label error\n To merge this PR, it requires {{ errorString }} {{ count }} label indicating the type of PR. Other labels are optional and not being checked here. \n- **Type-related labels to choose from**: {{ provided }}.\n- **Labels found**: {{ applied }}."
exit_type: success
4 changes: 4 additions & 0 deletions packages/components/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@

- `Modal`: Fix loss of focus when clicking outside ([#52653](https://github.com/WordPress/gutenberg/pull/52653)).

### Internal

- `TabPanel`: Introduce a new version of `TabPanel` with updated internals while maintaining the same functionality and API surface ([#52133](https://github.com/WordPress/gutenberg/pull/52133)).
chad1008 marked this conversation as resolved.
Show resolved Hide resolved

## 25.4.0 (2023-07-20)

### Enhancements
Expand Down
208 changes: 123 additions & 85 deletions packages/components/src/tab-panel/index.tsx
Original file line number Diff line number Diff line change
@@ -1,46 +1,39 @@
/**
* External dependencies
*/
import classnames from 'classnames';
import * as Ariakit from '@ariakit/react';
import cx from 'classnames';
chad1008 marked this conversation as resolved.
Show resolved Hide resolved
import type { ForwardedRef } from 'react';

/**
* WordPress dependencies
*/
import {
forwardRef,
useState,
useEffect,
useLayoutEffect,
useCallback,
} from '@wordpress/element';
import { useInstanceId } from '@wordpress/compose';
import { useInstanceId, usePrevious } from '@wordpress/compose';

/**
* Internal dependencies
*/
import { NavigableMenu } from '../navigable-container';

import Button from '../button';
import type { TabButtonProps, TabPanelProps } from './types';
import type { TabPanelProps } from './types';
import type { WordPressComponentProps } from '../ui/context';

const TabButton = ( {
tabId,
children,
selected,
...rest
}: TabButtonProps ) => (
<Button
role="tab"
tabIndex={ selected ? undefined : -1 }
aria-selected={ selected }
id={ tabId }
__experimentalIsFocusable
{ ...rest }
>
{ children }
</Button>
);
// Separate the actual tab name from the instance ID. This is
// necessary because Ariakit internally uses the element ID when
// a new tab is selected, but our implementation looks specifically
// for the tab name to be passed to the `onSelect` callback.
const extractTabName = ( id: string | undefined | null ) => {
if ( typeof id === 'undefined' || id === null ) {
return;
}
return id.match( /^tab-panel-[0-9]*-(.*)/ )?.[ 1 ];
};

/**
* TabPanel is an ARIA-compliant tabpanel.
Expand Down Expand Up @@ -92,112 +85,157 @@ const UnforwardedTabPanel = (
ref: ForwardedRef< any >
) => {
const instanceId = useInstanceId( TabPanel, 'tab-panel' );
const [ selected, setSelected ] = useState< string >();

const handleTabSelection = useCallback(
( tabKey: string ) => {
setSelected( tabKey );
onSelect?.( tabKey );
const prependInstanceId = useCallback(
( tabName: string | undefined ) => {
if ( typeof tabName === 'undefined' ) {
return;
}
return `${ instanceId }-${ tabName }`;
},
[ instanceId ]
);

const tabStore = Ariakit.useTabStore( {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just having a look at this PR — is there a reason why we imported * from Ariakit instead of importing only the needed components / functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, actually. Importing just the needed items would have been better. I'll plan a followup.

Copy link
Member

Choose a reason for hiding this comment

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

Just in case this is a factor, besides code style/aesthetics, there's no difference between importing the namespace and components individually. In the docs, we sometimes use the namespace import. The reasons are described in this section of the docs: https://ariakit.org/guide/coding-guidelines#import-namespace

setSelectedId: ( newTabValue ) => {
if ( typeof newTabValue === 'undefined' || newTabValue === null ) {
return;
}

const newTab = tabs.find(
( t ) => prependInstanceId( t.name ) === newTabValue
);
if ( newTab?.disabled || newTab === selectedTab ) {
return;
}

const simplifiedTabName = extractTabName( newTabValue );
if ( typeof simplifiedTabName === 'undefined' ) {
return;
}

onSelect?.( simplifiedTabName );
},
orientation,
selectOnMove,
defaultSelectedId: prependInstanceId( initialTabName ),
} );

const selectedTabName = extractTabName( tabStore.useState( 'selectedId' ) );

const setTabStoreSelectedId = useCallback(
( tabName: string ) => {
tabStore.setState( 'selectedId', prependInstanceId( tabName ) );
},
[ onSelect ]
[ prependInstanceId, tabStore ]
);

// Simulate a click on the newly focused tab, which causes the component
// to show the `tab-panel` associated with the clicked tab.
const activateTabAutomatically = (
_childIndex: number,
child: HTMLElement
) => {
child.click();
};
const selectedTab = tabs.find( ( { name } ) => name === selected );
const selectedId = `${ instanceId }-${ selectedTab?.name ?? 'none' }`;
const selectedTab = tabs.find( ( { name } ) => name === selectedTabName );

const previousSelectedTabName = usePrevious( selectedTabName );

// Ensure `onSelect` is called when the initial tab is selected.
useEffect( () => {
if (
previousSelectedTabName !== selectedTabName &&
selectedTabName === initialTabName &&
!! selectedTabName
) {
onSelect?.( selectedTabName );
}
}, [ selectedTabName, initialTabName, onSelect, previousSelectedTabName ] );

// Handle selecting the initial tab.
useLayoutEffect( () => {
// If there's a selected tab, don't override it.
if ( selectedTab ) {
return;
}

const initialTab = tabs.find( ( tab ) => tab.name === initialTabName );

// Wait for the denoted initial tab to be declared before making a
// selection. This ensures that if a tab is declared lazily it can
// still receive initial selection.
if ( initialTabName && ! initialTab ) {
return;
}

if ( initialTab && ! initialTab.disabled ) {
// Select the initial tab if it's not disabled.
handleTabSelection( initialTab.name );
setTabStoreSelectedId( initialTab.name );
} else {
// Fallback to the first enabled tab when the initial is disabled.
// Fallback to the first enabled tab when the initial tab is
// disabled or it can't be found.
const firstEnabledTab = tabs.find( ( tab ) => ! tab.disabled );
if ( firstEnabledTab ) handleTabSelection( firstEnabledTab.name );
if ( firstEnabledTab ) {
setTabStoreSelectedId( firstEnabledTab.name );
}
}
}, [ tabs, selectedTab, initialTabName, handleTabSelection ] );
}, [
tabs,
selectedTab,
initialTabName,
instanceId,
setTabStoreSelectedId,
] );

// Handle the currently selected tab becoming disabled.
useEffect( () => {
// This effect only runs when the selected tab is defined and becomes disabled.
if ( ! selectedTab?.disabled ) {
return;
}

const firstEnabledTab = tabs.find( ( tab ) => ! tab.disabled );

// If the currently selected tab becomes disabled, select the first enabled tab.
// (if there is one).
if ( firstEnabledTab ) {
handleTabSelection( firstEnabledTab.name );
setTabStoreSelectedId( firstEnabledTab.name );
}
}, [ tabs, selectedTab?.disabled, handleTabSelection ] );

}, [ tabs, selectedTab?.disabled, setTabStoreSelectedId, instanceId ] );
return (
<div className={ className } ref={ ref }>
<NavigableMenu
role="tablist"
orientation={ orientation }
onNavigate={
selectOnMove ? activateTabAutomatically : undefined
}
<Ariakit.TabList
store={ tabStore }
className="components-tab-panel__tabs"
>
{ tabs.map( ( tab ) => (
<TabButton
className={ classnames(
'components-tab-panel__tabs-item',
tab.className,
{
[ activeClass ]: tab.name === selected,
{ tabs.map( ( tab ) => {
return (
<Ariakit.Tab
key={ tab.name }
id={ prependInstanceId( tab.name ) }
className={ cx(
'components-tab-panel__tabs-item',
tab.className,
{
[ activeClass ]:
tab.name === selectedTabName,
}
) }
disabled={ tab.disabled }
aria-controls={ `${ prependInstanceId(
tab.name
) }-view` }
Comment on lines +213 to +215
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current TabPanel component adds aria-controls attributes to all of the Tabs, even though with lazy loading only the currently active tab was actually rendered. Ariakit was only adding this attribute for the current tab, because technically it's the only one that exists. I've manually added this in for consistency to match existing snapshots as closely as possible, but we can easily remove it and update the snapshots to expect the new behavior instead.

render={
<Button
icon={ tab.icon }
label={ tab.icon && tab.title }
showTooltip={ !! tab.icon }
/>
}
) }
tabId={ `${ instanceId }-${ tab.name }` }
aria-controls={ `${ instanceId }-${ tab.name }-view` }
selected={ tab.name === selected }
key={ tab.name }
onClick={ () => handleTabSelection( tab.name ) }
disabled={ tab.disabled }
label={ tab.icon && tab.title }
icon={ tab.icon }
showTooltip={ !! tab.icon }
>
{ ! tab.icon && tab.title }
</TabButton>
) ) }
</NavigableMenu>
>
{ ! tab.icon && tab.title }
</Ariakit.Tab>
);
} ) }
</Ariakit.TabList>
{ selectedTab && (
<div
key={ selectedId }
aria-labelledby={ selectedId }
role="tabpanel"
id={ `${ selectedId }-view` }
className="components-tab-panel__tab-content"
<Ariakit.TabPanel
id={ `${ prependInstanceId( selectedTab.name ) }-view` }
store={ tabStore }
key={ selectedTab.name }
chad1008 marked this conversation as resolved.
Show resolved Hide resolved
tabId={ prependInstanceId( selectedTab.name ) }
className={ 'components-tab-panel__tab-content' }
>
{ children( selectedTab ) }
</div>
</Ariakit.TabPanel>
) }
</div>
);
Expand Down
6 changes: 6 additions & 0 deletions packages/components/src/tab-panel/stories/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -96,3 +96,9 @@ WithTabIconsAndTooltips.args = {
},
],
};

export const ManualActivation = Template.bind( {} );
ManualActivation.args = {
...Default.args,
selectOnMove: false,
};
Loading