Steve-Dee-Designs
/
woocommerce
mirror of https://github.com/woocommerce/woocommerce.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Product Editor: Fix tab switching when variations are generated (#49058) 

* Remove selected tab state from Tabs, handle via props

* Pass selected tab via props to Header

* Fix Tabs unit tests

* Remove unused imports

* Changelog
This commit is contained in:
Matt Sherman 2024-07-04 09:58:26 -04:00 committed by GitHub
parent d95b43a6f5
commit d66e1a2438
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
6 changed files with 81 additions and 158 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
packages/js/product-editor/changelog/fix-create-variations-tab-switch Normal file
View File

@ -0,0 +1,4 @@
Significance: patch
Type: fix
Fixes a React 18 concurrency-related issue with tab switching.

10
packages/js/product-editor/src/components/editor/editor.tsx
View File

@ -5,12 +5,14 @@ import {
createElement,
StrictMode,
Fragment,
useCallback,
useState,
} from '@wordpress/element';
import {
LayoutContextProvider,
useExtendLayout,
} from '@woocommerce/admin-layout';
import { navigateTo, getNewPath, getQuery } from '@woocommerce/navigation';
import { useSelect } from '@wordpress/data';
import { Popover } from '@wordpress/components';
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
@ -39,7 +41,13 @@ import { PrepublishPanel } from '../prepublish-panel/prepublish-panel';
export function Editor( { productId, postType = 'product' }: EditorProps ) {
const [ isEditorLoading, setIsEditorLoading ] = useState( true );
const [ selectedTab, setSelectedTab ] = useState< string | null >( null );
const query = getQuery() as Record< string, string >;
const selectedTab = query.tab || null;
const setSelectedTab = useCallback( ( tabId: string ) => {
navigateTo( { url: getNewPath( { tab: tabId } ) } );
}, [] );
const updatedLayoutContext = useExtendLayout( 'product-block-editor' );

2
packages/js/product-editor/src/components/header/header.tsx
View File

@ -278,7 +278,7 @@ export function Header( {
<MoreMenu />
</div>
</div>
<Tabs onChange={ onTabSelect } />
<Tabs selected={ selectedTab } onChange={ onTabSelect } />
</div>
);
}

4
packages/js/product-editor/src/components/header/types.ts
View File

@ -1,7 +1,7 @@
export type HeaderProps = {
onTabSelect: ( tabId: string | null ) => void;
onTabSelect: ( tabId: string ) => void;
productType?: string;
selectedTab?: string | null;
selectedTab: string | null;
};
export interface Image {

70
packages/js/product-editor/src/components/tabs/tabs.tsx
View File

@ -1,22 +1,13 @@
/**
* External dependencies
*/
import {
createElement,
useEffect,
useState,
Fragment,
} from '@wordpress/element';
import { createElement, useEffect, Fragment } from '@wordpress/element';
import { KeyboardEvent, ReactElement, useMemo } from 'react';
import { NavigableMenu, Slot } from '@wordpress/components';
import { Product } from '@woocommerce/data';
import { recordEvent } from '@woocommerce/tracks';
import { select } from '@wordpress/data';
import { useEntityProp } from '@wordpress/core-data';
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore No types for this exist yet.
// eslint-disable-next-line @woocommerce/dependency-group
import { navigateTo, getNewPath, getQuery } from '@woocommerce/navigation';
/**
* Internal dependencies
@ -25,7 +16,8 @@ import { getTabTracksData } from './utils/get-tab-tracks-data';
import { TABS_SLOT_NAME } from './constants';
type TabsProps = {
onChange?: ( tabId: string | null ) => void;
selected: string | null;
onChange: ( tabId: string ) => void;
};
export type TabsFillProps = {
@ -34,10 +26,12 @@ export type TabsFillProps = {
function TabFills( {
fills,
onDefaultSelection,
selected,
onChange,
}: {
fills: readonly ( readonly ReactElement[] )[];
onDefaultSelection( tabId: string ): void;
selected: string | null;
onChange: ( tabId: string ) => void;
} ) {
const sortedFills = useMemo(
function sortFillsByOrder() {
@ -49,37 +43,34 @@ function TabFills( {
);
useEffect( () => {
for ( let i = 0; i < sortedFills.length; i++ ) {
const [ { props } ] = fills[ i ];
if ( ! props.disabled ) {
const tabId = props.children?.key;
if ( tabId ) {
onDefaultSelection( tabId );
}
return;
}
// If a tab is already selected, do nothing
if ( selected ) {
return;
}
}, [ sortedFills ] );
// Select the first tab that is not disabled
const firstEnabledTab = sortedFills.find( ( element ) => {
const [ { props } ] = element;
return ! props.disabled;
} );
const tabIdToSelect = firstEnabledTab?.[ 0 ]?.props?.children?.key;
if ( tabIdToSelect ) {
onChange( tabIdToSelect );
}
}, [ sortedFills, selected, onChange ] );
return <>{ sortedFills }</>;
}
export function Tabs( { onChange = () => {} }: TabsProps ) {
const [ selected, setSelected ] = useState< string | null >( null );
const query = getQuery() as Record< string, string >;
export function Tabs( { selected, onChange }: TabsProps ) {
const [ productId ] = useEntityProp< number >(
'postType',
'product',
'id'
);
useEffect( () => {
if ( query.tab ) {
setSelected( query.tab );
onChange( query.tab );
}
}, [ query.tab ] );
function selectTabOnNavigate(
_childIndex: number,
child: HTMLButtonElement
@ -115,13 +106,8 @@ export function Tabs( { onChange = () => {} }: TabsProps ) {
return (
<TabFills
fills={ fills }
onDefaultSelection={ ( tabId ) => {
if ( selected ) {
return;
}
setSelected( tabId );
onChange( tabId );
} }
selected={ selected }
onChange={ onChange }
/>
);
}
@ -138,9 +124,7 @@ export function Tabs( { onChange = () => {} }: TabsProps ) {
fillProps={
{
onClick: ( tabId ) => {
navigateTo( {
url: getNewPath( { tab: tabId } ),
} );
onChange( tabId );
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore

149
packages/js/product-editor/src/components/tabs/test/tabs.spec.tsx
View File

@ -3,9 +3,8 @@
*/
import React from 'react';
import { render, fireEvent, screen } from '@testing-library/react';
import { getQuery, navigateTo } from '@woocommerce/navigation';
import { SlotFillProvider } from '@wordpress/components';
import { useState, createElement } from '@wordpress/element';
import { createElement } from '@wordpress/element';
import { recordEvent } from '@woocommerce/tracks';
import { select } from '@wordpress/data';
@ -13,10 +12,7 @@ import { select } from '@wordpress/data';
* Internal dependencies
*/
import { Tabs } from '../';
import {
TabBlockEdit as Tab,
TabBlockAttributes,
} from '../../../blocks/generic/tab/edit';
import { TabBlockEdit as Tab } from '../../../blocks/generic/tab/edit';
import { TRACKS_SOURCE } from '../../../constants';
jest.mock( '@woocommerce/block-templates', () => ( {
@ -50,8 +46,13 @@ const blockProps = {
isSelected: false,
};
function MockTabs( { onChange = jest.fn() } ) {
const [ selected, setSelected ] = useState< string | null >( null );
function MockTabs( {
selected = null,
onChange = jest.fn(),
}: {
selected?: string | null;
onChange?: ( tabId: string ) => void;
} ) {
const mockContext = {
editedProduct: null,
postId: 1,
@ -59,24 +60,9 @@ function MockTabs( { onChange = jest.fn() } ) {
selectedTab: selected,
};
function getAttributes( id: string ) {
return function setAttributes( {
isSelected,
}: Partial< TabBlockAttributes > ) {
if ( isSelected ) {
setSelected( id );
}
};
}
return (
<SlotFillProvider>
<Tabs
onChange={ ( tabId ) => {
setSelected( tabId );
onChange( tabId );
} }
/>
<Tabs selected={ selected } onChange={ onChange } />
<Tab
{ ...blockProps }
attributes={ {
@ -85,11 +71,8 @@ function MockTabs( { onChange = jest.fn() } ) {
order: 1,
isSelected: selected === 'test1',
} }
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore editedProduct is not used, so we can just ignore the fact that our context doesn't have it
context={ mockContext }
name="test1"
setAttributes={ getAttributes( 'test1' ) }
/>
<Tab
{ ...blockProps }
@ -99,11 +82,8 @@ function MockTabs( { onChange = jest.fn() } ) {
order: 2,
isSelected: selected === 'test2',
} }
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore editedProduct is not used, so we can just ignore the fact that our context doesn't have it
context={ mockContext }
name="test2"
setAttributes={ getAttributes( 'test2' ) }
/>
<Tab
{ ...blockProps }
@ -113,11 +93,8 @@ function MockTabs( { onChange = jest.fn() } ) {
order: 3,
isSelected: selected === 'test3',
} }
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore editedProduct is not used, so we can just ignore the fact that our context doesn't have it
context={ mockContext }
name="test3"
setAttributes={ getAttributes( 'test3' ) }
/>
</SlotFillProvider>
);
@ -126,9 +103,6 @@ function MockTabs( { onChange = jest.fn() } ) {
describe( 'Tabs', () => {
beforeEach( () => {
jest.clearAllMocks();
( getQuery as jest.Mock ).mockReturnValue( {
tab: null,
} );
} );
it( 'should render tab buttons added to the slot', () => {
@ -138,93 +112,46 @@ describe( 'Tabs', () => {
expect( screen.queryByText( 'Test button 2' ) ).toBeInTheDocument();
} );
it( 'should set the first tab as active initially', async () => {
render( <MockTabs /> );
it( 'should call onChange with the first tab if no selection set', async () => {
const mockOnChange = jest.fn();
render( <MockTabs onChange={ mockOnChange } /> );
expect( mockOnChange ).toHaveBeenCalledWith( 'test1' );
} );
it( 'should set the selected tab active', async () => {
const mockOnChange = jest.fn();
render( <MockTabs selected={ 'test2' } onChange={ mockOnChange } /> );
expect( screen.queryByText( 'Test button 1' ) ).toHaveAttribute(
'aria-selected',
'true'
'false'
);
expect( screen.queryByText( 'Test button 2' ) ).toHaveAttribute(
'aria-selected',
'false'
'true'
);
expect( mockOnChange ).not.toHaveBeenCalled();
} );
it( 'should navigate to a new URL when a tab is clicked', () => {
render( <MockTabs /> );
it( 'should call the onChange prop when changing', async () => {
const mockOnChange = jest.fn();
const button = screen.getByText( 'Test button 2' );
render( <MockTabs selected={ 'test2' } onChange={ mockOnChange } /> );
const button = screen.getByText( 'Test button 1' );
fireEvent.click( button );
expect( navigateTo ).toHaveBeenLastCalledWith( {
url: 'admin.php?page=wc-admin&tab=test2',
} );
} );
it( 'should select the tab provided in the URL initially', () => {
( getQuery as jest.Mock ).mockReturnValue( {
tab: 'test2',
} );
render( <MockTabs /> );
expect( screen.getByText( 'Test button 2' ) ).toHaveAttribute(
'aria-selected',
'true'
);
} );
it( 'should select the tab provided on URL change', () => {
const { rerender } = render( <MockTabs /> );
( getQuery as jest.Mock ).mockReturnValue( {
tab: 'test3',
} );
rerender( <MockTabs /> );
expect( screen.getByText( 'Test button 3' ) ).toHaveAttribute(
'aria-selected',
'true'
);
} );
it( 'should call the onChange props when changing', async () => {
const mockOnChange = jest.fn();
const { rerender } = render( <MockTabs onChange={ mockOnChange } /> );
expect( mockOnChange ).toHaveBeenCalledWith( 'test1' );
( getQuery as jest.Mock ).mockReturnValue( {
tab: 'test2',
} );
rerender( <MockTabs onChange={ mockOnChange } /> );
expect( mockOnChange ).toHaveBeenCalledWith( 'test2' );
} );
it( 'should add a class to the initially selected tab panel', async () => {
render( <MockTabs /> );
const panel1 = screen.getByRole( 'tabpanel', {
name: 'Test button 1',
} );
const panel2 = screen.getByRole( 'tabpanel', {
name: 'Test button 2',
} );
expect( panel1.classList ).toContain( 'is-selected' );
expect( panel2.classList ).not.toContain( 'is-selected' );
} );
it( 'should add a class to the newly selected tab panel', async () => {
const { rerender } = render( <MockTabs /> );
const { rerender } = render( <MockTabs selected={ 'test2' } /> );
const button = screen.getByText( 'Test button 2' );
fireEvent.click( button );
const panel1 = screen.getByRole( 'tabpanel', {
name: 'Test button 1',
} );
@ -232,14 +159,13 @@ describe( 'Tabs', () => {
name: 'Test button 2',
} );
( getQuery as jest.Mock ).mockReturnValue( {
tab: 'test2',
} );
rerender( <MockTabs /> );
expect( panel1.classList ).not.toContain( 'is-selected' );
expect( panel2.classList ).toContain( 'is-selected' );
rerender( <MockTabs selected={ 'test1' } /> );
expect( panel1.classList ).toContain( 'is-selected' );
expect( panel2.classList ).not.toContain( 'is-selected' );
} );
it( 'should trigger wcadmin_product_tab_click track event when tab is clicked', async () => {
@ -252,6 +178,7 @@ describe( 'Tabs', () => {
const button = screen.getByText( 'Test button 2' );
fireEvent.click( button );
expect( recordEvent ).toBeCalledWith( 'product_tab_click', {
product_tab: 'test2',
product_type: 'simple',