-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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.
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
[@mantine/core] [docs] Fix allowStepSelect regression #3340
[@mantine/core] [docs] Fix allowStepSelect regression #3340
Conversation
return false; | ||
} | ||
|
||
if (typeof item.props.allowStepSelect === 'boolean') { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
}; | ||
|
||
// 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.
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!
Thanks! |
This is my first contribution to the repo so feel free to roast me if something isn't up to the standards :)