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

[@mantine/core] [docs] Fix allowStepSelect regression #3340

Merged
merged 2 commits into from
Jan 17, 2023
Merged

[@mantine/core] [docs] Fix allowStepSelect regression #3340

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
89a6454
[@mantine/core] Stepper: fix allowStepSelect regression
rmzNadir Jan 11, 2023
431cf47
[docs] Update Stepper docs
rmzNadir Jan 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/src/docs/core/Stepper.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import { StepperDemos } from '@mantine/demos';
## Allow step select

To disable step selection set `allowStepSelect` prop on `Stepper.Step` component.
It can be used to prevent user from reaching next steps:
It can be used to prevent user from reaching next steps while letting them go back and forth between steps they've already reached before:

<Demo data={StepperDemos.allowStepSelect} />

Expand Down
63 changes: 63 additions & 0 deletions src/mantine-core/src/Stepper/Stepper.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,4 +57,67 @@ describe('@mantine/core/Stepper', () => {
expect(Stepper.Step).toBe(Step);
expect(Stepper.Completed).toBe(StepCompleted);
});

it('allows bidirectional selection between steps by default', async () => {
const spy = jest.fn();
render(<Stepper {...defaultProps} onStepClick={spy} />);

const stepButtons = screen.getAllByRole('button');

await userEvent.click(stepButtons[3]);

expect(spy).toHaveBeenCalledTimes(1);
expect(spy).toHaveBeenCalledWith(3);

await userEvent.click(stepButtons[0]);

expect(spy).toHaveBeenCalledTimes(2);
expect(spy).toHaveBeenCalledWith(0);
});

it('only allows selecting previous steps if the allowNextStepsSelect prop is set to false and no truthy allowStepSelectprop is present on any steps', async () => {
const spy = jest.fn();
render(<Stepper {...defaultProps} onStepClick={spy} allowNextStepsSelect={false} />);

const stepButtons = screen.getAllByRole('button');

await userEvent.click(stepButtons[2]);

expect(spy).not.toHaveBeenCalled();

await userEvent.click(stepButtons[0]);

expect(spy).toHaveBeenCalledTimes(1);
expect(spy).toHaveBeenCalledWith(0);
});

it('allows any steps to be selected if a Step has the allowStepSelect prop set to true even if a falsy allowNextStepsSelect prop is present on the Stepper', async () => {
const spy = jest.fn();
render(
<Stepper onStepClick={spy} allowNextStepsSelect={false} active={0}>
<Stepper.Step label="0" key="0" description="0">
test-step-content-0
</Stepper.Step>

<Stepper.Step label="1" key="1" description="1">
test-step-content-1
</Stepper.Step>

<Stepper.Step label="2" key="2" description="2" allowStepSelect>
test-step-content-2
</Stepper.Step>
</Stepper>
);

const steps = screen.getAllByRole('button');

await userEvent.click(steps[1]);

expect(spy).not.toHaveBeenCalled();

await userEvent.click(steps[2]);

expect(spy).toHaveBeenCalledTimes(1);
expect(spy).toHaveBeenCalledWith(2);
});
});
22 changes: 16 additions & 6 deletions src/mantine-core/src/Stepper/Stepper.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,10 +117,20 @@ export const Stepper: StepperComponent = forwardRef<HTMLDivElement, StepperProps
const items = _children.reduce<React.ReactElement[]>((acc, item, index) => {
const state =
active === index ? 'stepProgress' : active > index ? 'stepCompleted' : 'stepInactive';
const shouldAllowSelect = state === 'stepCompleted' || allowNextStepsSelect;
typeof item.props.allowStepSelect === 'boolean'
? item.props.allowStepSelect
: typeof onStepClick === 'function';

const shouldAllowSelect = () => {
if (typeof onStepClick !== 'function') {
return false;
}

if (typeof item.props.allowStepSelect === 'boolean') {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Giving priority to the Step's allowStepSelect prop made sense for more fine grain control over the step selection.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, and here was actually the typo I added in the previous code... I forgot a && operator, with the "typeof item.props..." part staying alone on its own line for nothing.

Copy link
Contributor

Choose a reason for hiding this comment

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

In terms of readability I agree with your change; in terms of performance since this is in a loop, the added function call might not be the best idea. Then again, I don't imagine a stepper component getting tens or hundreds of steps :)

Copy link
Contributor

Choose a reason for hiding this comment

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

The following will achieve the same, without the function call:

    const defaultAllowSelect = state === 'stepCompleted' || allowNextStepsSelect;
    const shouldAllowSelect =
      defaultAllowSelect &&
      (typeof item.props.allowStepSelect === 'boolean'
        ? item.props.allowStepSelect
        : typeof onStepClick === 'function');

But it's arguably more difficult to read.

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 performance change is so negligible (even with a thousand steps) that it may as well be part of the margin of error and at that point I'd rather favor readability

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I agree. This was definitely a nitpick.

return item.props.allowStepSelect;
}

return state === 'stepCompleted' || allowNextStepsSelect;
};

const isStepSelectionEnabled = shouldAllowSelect();

acc.push(
cloneElement(item, {
Expand All @@ -129,8 +139,8 @@ export const Stepper: StepperComponent = forwardRef<HTMLDivElement, StepperProps
key: index,
step: index,
state,
onClick: () => shouldAllowSelect && typeof onStepClick === 'function' && onStepClick(index),
allowStepClick: shouldAllowSelect && typeof onStepClick === 'function',
onClick: () => isStepSelectionEnabled && onStepClick(index),
allowStepClick: isStepSelectionEnabled,
completedIcon: item.props.completedIcon || completedIcon,
progressIcon: item.props.progressIcon || progressIcon,
color: item.props.color || color,
Expand Down
85 changes: 67 additions & 18 deletions src/mantine-demos/src/demos/core/Stepper/Stepper.demo.allowStepSelect.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,29 +9,57 @@ import { Stepper, Button, Group } from '@mantine/core';

function Demo() {
const [active, setActive] = useState(1);
const nextStep = () => setActive((current) => (current < 3 ? current + 1 : current));
const prevStep = () => setActive((current) => (current > 0 ? current - 1 : current));
const [highestStepVisited, setHighestStepVisited] = useState(active);

const handleStepChange = (nextStep: number) => {
const isOutOfBounds = nextStep > 3 || nextStep < 0;

if (isOutOfBounds) {
return;
}

setActive(nextStep);
setHighestStepVisited((hSC) => Math.max(hSC, nextStep));
};

// Allow the user to freely go back and forth between visited steps.
const shouldAllowSelectStep = (step: number) => highestStepVisited >= step && active !== step;
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this variant of the demo, good idea!


return (
<>
<Stepper active={active} onStepClick={setActive} breakpoint="sm">
<Stepper.Step label="First step" description="Create an account" allowStepSelect={active > 0}>
Step 1 content: Create an account
<Stepper.Step
label="First step"
description="Create an account"
allowStepSelect={shouldAllowSelectStep(0)}
>
<Content>Step 1 content: Create an account</Content>
</Stepper.Step>
<Stepper.Step label="Second step" description="Verify email" allowStepSelect={active > 1}>
Step 2 content: Verify email
<Stepper.Step
label="Second step"
description="Verify email"
allowStepSelect={shouldAllowSelectStep(1)}
>
<Content>Step 2 content: Verify email</Content>
</Stepper.Step>
<Stepper.Step label="Final step" description="Get full access" allowStepSelect={active > 2}>
Step 3 content: Get full access
<Stepper.Step
label="Final step"
description="Get full access"
allowStepSelect={shouldAllowSelectStep(2)}
>
<Content>Step 3 content: Get full access</Content>
</Stepper.Step>

<Stepper.Completed>
Completed, click back button to get to previous step
<Content>Completed, click back button to get to previous step</Content>
</Stepper.Completed>
</Stepper>

<Group position="center" mt="xl">
<Button variant="default" onClick={prevStep}>Back</Button>
<Button onClick={nextStep}>Next step</Button>
<Button variant="default" onClick={() => handleStepChange(active - 1)}>
Back
</Button>
<Button onClick={() => handleStepChange(active + 1)}>Next step</Button>
</Group>
</>
);
Expand All @@ -40,23 +68,44 @@ function Demo() {

function Demo() {
const [active, setActive] = useState(1);
const nextStep = () => setActive((current) => (current < 3 ? current + 1 : current));
const prevStep = () => setActive((current) => (current > 0 ? current - 1 : current));
const [highestStepVisited, setHighestStepVisited] = useState(active);

const handleStepChange = (nextStep: number) => {
const isOutOfBounds = nextStep > 3 || nextStep < 0;

if (isOutOfBounds) {
return;
}

setActive(nextStep);
setHighestStepVisited((hSC) => Math.max(hSC, nextStep));
};

// Allow the user to freely go back and forth between visited steps.
const shouldAllowSelectStep = (step: number) => highestStepVisited >= step && active !== step;

return (
<>
<Stepper active={active} onStepClick={setActive} breakpoint="sm">
<Stepper.Step
label="First step"
description="Create an account"
allowStepSelect={active > 0}
allowStepSelect={shouldAllowSelectStep(0)}
>
<Content>Step 1 content: Create an account</Content>
</Stepper.Step>
<Stepper.Step label="Second step" description="Verify email" allowStepSelect={active > 1}>
<Stepper.Step
label="Second step"
description="Verify email"
allowStepSelect={shouldAllowSelectStep(1)}
>
<Content>Step 2 content: Verify email</Content>
</Stepper.Step>
<Stepper.Step label="Final step" description="Get full access" allowStepSelect={active > 2}>
<Stepper.Step
label="Final step"
description="Get full access"
allowStepSelect={shouldAllowSelectStep(2)}
>
<Content>Step 3 content: Get full access</Content>
</Stepper.Step>

Expand All @@ -66,10 +115,10 @@ function Demo() {
</Stepper>

<Group position="center" mt="xl">
<Button variant="default" onClick={prevStep}>
<Button variant="default" onClick={() => handleStepChange(active - 1)}>
Back
</Button>
<Button onClick={nextStep}>Next step</Button>
<Button onClick={() => handleStepChange(active + 1)}>Next step</Button>
</Group>
</>
);
Expand Down