[@mantine/core] [docs] Fix allowStepSelect regression #3340
Conversation
| return false; | ||
| } | ||
|
|
||
| if (typeof item.props.allowStepSelect === 'boolean') { |
There was a problem hiding this comment.
Giving priority to the Step's allowStepSelect prop made sense for more fine grain control over the step selection.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Yeah I agree. This was definitely a nitpick.
| }; | ||
|
|
||
| // Allow the user to freely go back and forth between visited steps. | ||
| const shouldAllowSelectStep = (step: number) => highestStepVisited >= step && active !== step; |
There was a problem hiding this comment.
I like this variant of the demo, good idea!
|
Thanks! |
This is my first contribution to the repo so feel free to roast me if something isn't up to the standards :)