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
feat(useStepper): new function #1679
Conversation
@innocenzi I'd maybe rename this to "useStepper" or "useStepperWizard" ... what do you think? |
Sounds good to me yes! I'll do this on monday or earlier if I have the time this week-end. |
I think |
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 idea in general. But I would try to make it minimal as possible.
packages/core/useStepper/index.ts
Outdated
function backTo(step: T) { | ||
if (currentStepIsAfter(step)) | ||
goTo(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 would prefer to have this check on usage.
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'm sorry, I don't understand. On usage of what? To clarify, goTo
allows to jump to a step without any check, while backTo
allows to go back to a previous step if it has been completed.
I needed this feature when trying to allow to click on the step names, but only when it has been done, as you can see on the demo:
<button :disabled="todo(step)" @click="backTo(step)" v-text="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.
It would feel a bit weird to me that:
const s = useStepper([1,2,3,4])
s.goTo(2)
// ...
s.backTo(3)
// and s stays on 2
In your case, the disabled
already serve as a guard for it.
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.
But backTo
can be called from somewhere else than a button and in that case, disabled
wouldn't guard it. Also a button's attribute is easier to tamper with than the code directly.
If you think it's weird maybe you just need to use goTo
in such a situation? I designed backTo
with the guard idea in mind, and if it's removed, the function becomes useless in favor of goTo
. But in every wizard I made in my recent apps, this specific function was useful.
Maybe we can rename backTo
with a name that would be less confusing?
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, what I mean is the guard should be done on userland when calling goTo
. Since there is only one line of addition in backTo
, the trade-off of saving one line but introducing an additional abstraction layer that users/readers need to understand is a bit less worthy to me. I'd suggest we remove it, and let users define it when needed.
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 understand - I removed it.
That being said, I still think this function is very useful, since I used it in all my wizards without exception - and I updated the demo code to show you how an userland backTo
would look like, I think it's not the best DX-wise.
But I respect your decision if you still think it's not needed!
packages/core/useStepper/index.ts
Outdated
@@ -0,0 +1,83 @@ | |||
import { computed, ref } from 'vue-demi' | |||
|
|||
export function useStepper<T extends string>(steps: readonly T[], initial?: T) { |
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.
export function useStepper<T extends string>(steps: readonly T[], initial?: T) { | |
export function useStepper<T>(steps: readonly T[], initial?: T) { |
Can we have it arbitrary? I might want to use objects.
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.
Yes I don't see a reason why it should only be strings. I use that mostly but you're right that objects can be useful. Also added basic tests for that.
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.
@antfu Do you have a suggestion of how to use currentStepIs
and other functions with objects? Partial match?
I followed-up in #1754 with a better implementation. Closing this because the review is stale. |
useWizard
Description
This is a utility function that helps building step-based wizard components. This is something that I found myself doing over and over in our apps (at least 3 times), and I think it's quite useful.
The declaration of the steps is done via an array of
stringssteps of arbitrary types. The composable returns a few functions and computed variables that are useful for navigating through the steps.For instance, the
goToPrevious
andgoToNext
function check if we can actually go back or forward. Computed variables likeisFirst
orisLast
orcurrent
help determining what to display. To display the component for the current step, one can usecurrentStepIs('step name')
.I'll happily review the wording of every function and variable, but they are all useful ones that I actually used in the real world.
What is the purpose of this pull request?