From d66e1a243870fbc65c90999c922e44a23e8f6752 Mon Sep 17 00:00:00 2001 From: Matt Sherman Date: Thu, 4 Jul 2024 09:58:26 -0400 Subject: [PATCH] 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 --- .../fix-create-variations-tab-switch | 4 + .../src/components/editor/editor.tsx | 10 +- .../src/components/header/header.tsx | 2 +- .../src/components/header/types.ts | 4 +- .../src/components/tabs/tabs.tsx | 70 ++++---- .../src/components/tabs/test/tabs.spec.tsx | 149 +++++------------- 6 files changed, 81 insertions(+), 158 deletions(-) create mode 100644 packages/js/product-editor/changelog/fix-create-variations-tab-switch diff --git a/packages/js/product-editor/changelog/fix-create-variations-tab-switch b/packages/js/product-editor/changelog/fix-create-variations-tab-switch new file mode 100644 index 00000000000..378c853bf17 --- /dev/null +++ b/packages/js/product-editor/changelog/fix-create-variations-tab-switch @@ -0,0 +1,4 @@ +Significance: patch +Type: fix + +Fixes a React 18 concurrency-related issue with tab switching. diff --git a/packages/js/product-editor/src/components/editor/editor.tsx b/packages/js/product-editor/src/components/editor/editor.tsx index e6af172d89e..71aee041491 100644 --- a/packages/js/product-editor/src/components/editor/editor.tsx +++ b/packages/js/product-editor/src/components/editor/editor.tsx @@ -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' ); diff --git a/packages/js/product-editor/src/components/header/header.tsx b/packages/js/product-editor/src/components/header/header.tsx index f219835d707..aaa3b7b7251 100644 --- a/packages/js/product-editor/src/components/header/header.tsx +++ b/packages/js/product-editor/src/components/header/header.tsx @@ -278,7 +278,7 @@ export function Header( { - + ); } diff --git a/packages/js/product-editor/src/components/header/types.ts b/packages/js/product-editor/src/components/header/types.ts index ddcb565cace..382f9950ccb 100644 --- a/packages/js/product-editor/src/components/header/types.ts +++ b/packages/js/product-editor/src/components/header/types.ts @@ -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 { diff --git a/packages/js/product-editor/src/components/tabs/tabs.tsx b/packages/js/product-editor/src/components/tabs/tabs.tsx index 9785684c24d..0adb5c3f8ac 100644 --- a/packages/js/product-editor/src/components/tabs/tabs.tsx +++ b/packages/js/product-editor/src/components/tabs/tabs.tsx @@ -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 ( { - 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 diff --git a/packages/js/product-editor/src/components/tabs/test/tabs.spec.tsx b/packages/js/product-editor/src/components/tabs/test/tabs.spec.tsx index 89f768e2ff2..ef9c3bda571 100644 --- a/packages/js/product-editor/src/components/tabs/test/tabs.spec.tsx +++ b/packages/js/product-editor/src/components/tabs/test/tabs.spec.tsx @@ -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 ( - { - setSelected( tabId ); - onChange( tabId ); - } } - /> + ); @@ -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( ); + it( 'should call onChange with the first tab if no selection set', async () => { + const mockOnChange = jest.fn(); + + render( ); + + expect( mockOnChange ).toHaveBeenCalledWith( 'test1' ); + } ); + + it( 'should set the selected tab active', async () => { + const mockOnChange = jest.fn(); + + render( ); 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( ); + it( 'should call the onChange prop when changing', async () => { + const mockOnChange = jest.fn(); - const button = screen.getByText( 'Test button 2' ); + render( ); + + 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( ); - - expect( screen.getByText( 'Test button 2' ) ).toHaveAttribute( - 'aria-selected', - 'true' - ); - } ); - - it( 'should select the tab provided on URL change', () => { - const { rerender } = render( ); - - ( getQuery as jest.Mock ).mockReturnValue( { - tab: 'test3', - } ); - - rerender( ); - - 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( ); - expect( mockOnChange ).toHaveBeenCalledWith( 'test1' ); - - ( getQuery as jest.Mock ).mockReturnValue( { - tab: 'test2', - } ); - - rerender( ); - - expect( mockOnChange ).toHaveBeenCalledWith( 'test2' ); - } ); - - it( 'should add a class to the initially selected tab panel', async () => { - render( ); - - 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( ); + const { rerender } = render( ); - 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( ); - expect( panel1.classList ).not.toContain( 'is-selected' ); expect( panel2.classList ).toContain( 'is-selected' ); + + rerender( ); + + 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',