From 34658c1bfa346e9e28082963a462468b493c37c6 Mon Sep 17 00:00:00 2001 From: Jeff Puzzo Date: Fri, 13 Oct 2023 18:13:28 -0400 Subject: [PATCH] fix(Wizard): onStepChange - skip isDisabled & isHidden --- .../src/components/Wizard/Wizard.tsx | 24 +-- .../src/components/Wizard/WizardBody.tsx | 4 +- .../components/Wizard/WizardNavInternal.tsx | 4 +- .../src/components/Wizard/WizardToggle.tsx | 2 +- .../Wizard/__tests__/Wizard.test.tsx | 184 ++++++++++++++++-- .../Wizard/__tests__/WizardBody.test.tsx | 2 +- .../Wizard/examples/WizardGetCurrentStep.tsx | 42 +++- .../react-core/src/components/Wizard/utils.ts | 27 ++- 8 files changed, 242 insertions(+), 47 deletions(-) diff --git a/packages/react-core/src/components/Wizard/Wizard.tsx b/packages/react-core/src/components/Wizard/Wizard.tsx index e12e513ca83..78a90a0cf20 100644 --- a/packages/react-core/src/components/Wizard/Wizard.tsx +++ b/packages/react-core/src/components/Wizard/Wizard.tsx @@ -11,7 +11,7 @@ import { WizardNavType, WizardStepChangeScope } from './types'; -import { buildSteps } from './utils'; +import { buildSteps, isStepEnabled } from './utils'; import { useWizardContext, WizardContextProvider } from './WizardContext'; import { WizardToggle } from './WizardToggle'; import { WizardNavInternal } from './WizardNavInternal'; @@ -23,7 +23,7 @@ import { WizardNavInternal } from './WizardNavInternal'; export interface WizardProps extends React.HTMLProps { /** Step components */ - children: React.ReactNode | React.ReactNode[]; + children: React.ReactNode; /** Wizard header */ header?: React.ReactNode; /** Wizard footer */ @@ -86,35 +86,23 @@ export const Wizard = ({ }, [startIndex]); const goToNextStep = (event: React.MouseEvent, steps: WizardStepType[] = initialSteps) => { - const newStep = steps.find( - (step) => step.index > activeStepIndex && !step.isHidden && !step.isDisabled && !isWizardParentStep(step) - ); + const newStep = steps.find((step) => step.index > activeStepIndex && isStepEnabled(steps, step)); if (activeStepIndex >= steps.length || !newStep?.index) { return onSave ? onSave(event) : onClose?.(event); } - const currStep = isWizardParentStep(steps[activeStepIndex]) ? steps[activeStepIndex + 1] : steps[activeStepIndex]; - const prevStep = steps[activeStepIndex - 1]; - setActiveStepIndex(newStep?.index); - onStepChange?.(event, currStep, prevStep, WizardStepChangeScope.Next); + onStepChange?.(event, newStep, steps[activeStepIndex - 1], WizardStepChangeScope.Next); }; const goToPrevStep = (event: React.MouseEvent, steps: WizardStepType[] = initialSteps) => { const newStep = [...steps] .reverse() - .find( - (step: WizardStepType) => - step.index < activeStepIndex && !step.isHidden && !step.isDisabled && !isWizardParentStep(step) - ); - const currStep = isWizardParentStep(steps[activeStepIndex - 2]) - ? steps[activeStepIndex - 3] - : steps[activeStepIndex - 2]; - const prevStep = steps[activeStepIndex - 1]; + .find((step: WizardStepType) => step.index < activeStepIndex && isStepEnabled(steps, step)); setActiveStepIndex(newStep?.index); - onStepChange?.(event, currStep, prevStep, WizardStepChangeScope.Back); + onStepChange?.(event, newStep, steps[activeStepIndex - 1], WizardStepChangeScope.Back); }; const goToStepByIndex = ( diff --git a/packages/react-core/src/components/Wizard/WizardBody.tsx b/packages/react-core/src/components/Wizard/WizardBody.tsx index 958bca7c401..5f68c179483 100644 --- a/packages/react-core/src/components/Wizard/WizardBody.tsx +++ b/packages/react-core/src/components/Wizard/WizardBody.tsx @@ -12,7 +12,7 @@ import { getResizeObserver } from '../../helpers/resizeObserver'; export interface WizardBodyProps { /** Anything that can be rendered in the Wizard body */ - children: React.ReactNode | React.ReactNode[]; + children: React.ReactNode; /** Flag to remove the default body padding */ hasNoPadding?: boolean; /** Adds an accessible name to the wrapper element when the content overflows and renders @@ -67,7 +67,7 @@ export const WizardBody = ({ return () => { observer(); }; - }, []); + }, [previousWidth]); return ( goToStepByIndex(subStep.index)} @@ -109,7 +109,7 @@ export const WizardNavInternal = ({ content={step.name} isExpandable={step.isExpandable} isCurrent={hasActiveChild} - isDisabled={!hasEnabledChildren} + isDisabled={!hasEnabledChildren || isStepDisabled} isVisited={step.isVisited} stepIndex={firstSubStepIndex} onClick={() => goToStepByIndex(firstSubStepIndex)} diff --git a/packages/react-core/src/components/Wizard/WizardToggle.tsx b/packages/react-core/src/components/Wizard/WizardToggle.tsx index 1f4a9de5985..cf815ffbb08 100644 --- a/packages/react-core/src/components/Wizard/WizardToggle.tsx +++ b/packages/react-core/src/components/Wizard/WizardToggle.tsx @@ -71,7 +71,7 @@ export const WizardToggle = ({ return ( - {activeStep?.name === step.name && + {activeStep?.id === step.id && (body || body === undefined ? {children} : children)}
diff --git a/packages/react-core/src/components/Wizard/__tests__/Wizard.test.tsx b/packages/react-core/src/components/Wizard/__tests__/Wizard.test.tsx index 81f0dd56446..e05f6f0050e 100644 --- a/packages/react-core/src/components/Wizard/__tests__/Wizard.test.tsx +++ b/packages/react-core/src/components/Wizard/__tests__/Wizard.test.tsx @@ -3,7 +3,7 @@ import React from 'react'; import { render, screen } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; -import { Wizard, WizardFooterProps, WizardStep, WizardNavProps } from '../'; +import { Wizard, WizardFooterProps, WizardStep, WizardNavProps, WizardStepChangeScope } from '../'; test('renders step when child is of type WizardStep', () => { render( @@ -100,7 +100,6 @@ test('renders default nav with custom props', () => { }); test('renders nav aria label', () => { - render( @@ -174,28 +173,33 @@ test(`can customize the wizard's height and width`, () => { expect(wizard).toHaveStyle('width: 500px'); }); -test('calls onNavByIndex on nav item click', async () => { +test('calls onStepChange on nav item click', async () => { const user = userEvent.setup(); - const onNavByIndex = jest.fn(); + const onStepChange = jest.fn(); render( - + ); await user.click(screen.getByRole('button', { name: 'Test step 2' })); - expect(onNavByIndex).toHaveBeenCalled(); + expect(onStepChange).toHaveBeenCalledWith( + null, + expect.objectContaining({ id: 'step-2' }), + expect.objectContaining({ id: 'step-1' }), + WizardStepChangeScope.Nav + ); }); -test('calls onNext and not onSave on next button click when not on the last step', async () => { +test('calls onStepChange and not onSave on next button click when not on the last step', async () => { const user = userEvent.setup(); - const onNext = jest.fn(); + const onStepChange = jest.fn(); const onSave = jest.fn(); render( - + @@ -203,16 +207,21 @@ test('calls onNext and not onSave on next button click when not on the last step await user.click(screen.getByRole('button', { name: 'Next' })); - expect(onNext).toHaveBeenCalled(); + expect(onStepChange).toHaveBeenCalledWith( + null, + expect.objectContaining({ id: 'step-2' }), + expect.objectContaining({ id: 'step-1' }), + WizardStepChangeScope.Next + ); expect(onSave).not.toHaveBeenCalled(); }); -test('calls onBack on back button click', async () => { +test('calls onStepChange on back button click', async () => { const user = userEvent.setup(); - const onBack = jest.fn(); + const onStepChange = jest.fn(); render( - + @@ -221,7 +230,12 @@ test('calls onBack on back button click', async () => { await user.click(screen.getByRole('button', { name: 'Test step 2' })); await user.click(screen.getByRole('button', { name: 'Back' })); - expect(onBack).toHaveBeenCalled(); + expect(onStepChange).toHaveBeenCalledWith( + null, + expect.objectContaining({ id: 'step-1' }), + expect.objectContaining({ id: 'step-2' }), + WizardStepChangeScope.Back + ); }); test('calls onSave and not onClose on next button click when on the last step', async () => { @@ -445,17 +459,153 @@ test('incrementally shows/hides steps based on the activeStep when isProgressive ).toBeNull(); }); -test('parent step can be non-collapsible by setting isCollapsible to false', () => { +test('parent step can be expandable by setting isExpandable to true', () => { render( ]} /> ); - expect(screen.queryByLabelText('step icon', { exact: false })).toBeNull(); + expect(screen.getByLabelText('step icon', { exact: false })).toBeVisible(); +}); + +test('child steps are disabled when parent is disabled', () => { + render( + + ]} + /> + + ); + + expect( + screen.getByRole('button', { + name: 'Test step 1' + }) + ).toBeDisabled(); + expect( + screen.getByRole('button', { + name: 'Sub step 1' + }) + ).toBeDisabled(); +}); + +test('child steps are hidden when parent is hidden', () => { + render( + + ]} /> + + ); + + expect( + screen.queryByRole('button', { + name: 'Test step 1' + }) + ).toBeNull(); + expect( + screen.queryByRole('button', { + name: 'Sub step 1' + }) + ).toBeNull(); +}); + +test('onStepChange skips over disabled or hidden steps and substeps', async () => { + const user = userEvent.setup(); + const onStepChange = jest.fn(); + + render( + + + ]} /> + + ]} + /> + , + + ]} + /> + + ]} + /> + , + + ]} + /> + + ); + + const nextButton = screen.getByRole('button', { name: 'Next' }); + const backButton = screen.getByRole('button', { name: 'Back' }); + + await user.click(nextButton); + expect(onStepChange).toHaveBeenCalledWith( + null, + expect.objectContaining({ id: 'step2-sub1' }), + expect.objectContaining({ id: 'step-1' }), + WizardStepChangeScope.Next + ); + + await user.click(nextButton); + expect(onStepChange).toHaveBeenCalledWith( + null, + expect.objectContaining({ id: 'step5-sub2' }), + expect.objectContaining({ id: 'step2-sub1' }), + WizardStepChangeScope.Next + ); + + await user.click(nextButton); + expect(onStepChange).toHaveBeenCalledWith( + null, + expect.objectContaining({ id: 'step8-sub2' }), + expect.objectContaining({ id: 'step5-sub2' }), + WizardStepChangeScope.Next + ); + + await user.click(backButton); + expect(onStepChange).toHaveBeenCalledWith( + null, + expect.objectContaining({ id: 'step5-sub2' }), + expect.objectContaining({ id: 'step8-sub2' }), + WizardStepChangeScope.Back + ); + + await user.click(backButton); + expect(onStepChange).toHaveBeenCalledWith( + null, + expect.objectContaining({ id: 'step2-sub1' }), + expect.objectContaining({ id: 'step5-sub2' }), + WizardStepChangeScope.Back + ); + + await user.click(backButton); + expect(onStepChange).toHaveBeenCalledWith( + null, + expect.objectContaining({ id: 'step-1' }), + expect.objectContaining({ id: 'step2-sub1' }), + WizardStepChangeScope.Back + ); }); diff --git a/packages/react-core/src/components/Wizard/__tests__/WizardBody.test.tsx b/packages/react-core/src/components/Wizard/__tests__/WizardBody.test.tsx index 1bf85474455..aa56f954c43 100644 --- a/packages/react-core/src/components/Wizard/__tests__/WizardBody.test.tsx +++ b/packages/react-core/src/components/Wizard/__tests__/WizardBody.test.tsx @@ -10,7 +10,7 @@ test('renders children without additional props', () => { expect(container).not.toHaveAttribute('aria-labelledby'); }); -test('has no padding className when hasNoBodyPadding is not specified', () => { +test('has no padding className when hasNoPadding is not specified', () => { render(content); expect(screen.getByText('content')).not.toHaveClass('pf-m-no-padding'); }); diff --git a/packages/react-core/src/components/Wizard/examples/WizardGetCurrentStep.tsx b/packages/react-core/src/components/Wizard/examples/WizardGetCurrentStep.tsx index af08ad5c599..af1eabb013f 100644 --- a/packages/react-core/src/components/Wizard/examples/WizardGetCurrentStep.tsx +++ b/packages/react-core/src/components/Wizard/examples/WizardGetCurrentStep.tsx @@ -36,7 +36,7 @@ const CurrentStepDescriptionList = ({ currentStep }: { currentStep: WizardStepTy export const GetCurrentStepWizard: React.FunctionComponent = () => { const [currentStep, setCurrentStep] = React.useState(); - const onStepChange = (event: React.MouseEvent, currentStep: WizardStepType) => + const onStepChange = (_event: React.MouseEvent, currentStep: WizardStepType) => setCurrentStep(currentStep); return ( @@ -44,9 +44,43 @@ export const GetCurrentStepWizard: React.FunctionComponent = () => { {currentStep ? : 'Step 1 content'} - - - + + + , + + + + ]} + /> + + + , + + + + ]} + /> + + + , + + + + ]} + /> diff --git a/packages/react-core/src/components/Wizard/utils.ts b/packages/react-core/src/components/Wizard/utils.ts index f83a42a0381..541b6b9eecd 100644 --- a/packages/react-core/src/components/Wizard/utils.ts +++ b/packages/react-core/src/components/Wizard/utils.ts @@ -1,6 +1,6 @@ import React from 'react'; -import { WizardStepType } from './types'; +import { isWizardParentStep, isWizardSubStep, WizardStepType } from './types'; import { WizardStep, WizardStepProps } from './WizardStep'; /** @@ -8,7 +8,7 @@ import { WizardStep, WizardStepProps } from './WizardStep'; * @param children * @returns WizardStepType[] */ -export const buildSteps = (children: React.ReactNode | React.ReactNode[]) => +export const buildSteps = (children: React.ReactNode) => React.Children.toArray(children).reduce((acc: WizardStepType[], child: React.ReactNode, index: number) => { if (isWizardStep(child)) { const { props: childProps } = child; @@ -57,3 +57,26 @@ export const normalizeStepProps = ({ steps: _steps, ...controlStep }: WizardStepProps): Omit => controlStep; + +/** + * Determines whether a step is navigable based on disabled/hidden properties + * @param steps All steps + * @param step + * @returns boolean + */ +export const isStepEnabled = (steps: WizardStepType[], step: WizardStepType): boolean => { + // Skip over parent steps since they are merely containers of sub-steps + if (!isWizardParentStep(step) && !step.isHidden && !step.isDisabled) { + if (isWizardSubStep(step)) { + const parentStep = steps.find((otherStep) => otherStep.id === step.parentId); + + if (!parentStep.isHidden && !parentStep.isDisabled) { + return true; + } + } else { + return true; + } + } + + return false; +};