-
Notifications
You must be signed in to change notification settings - Fork 30
feat(hooks): Adding useStepper hook [JOB-124608] #2567
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
base: master
Are you sure you want to change the base?
Conversation
Deploying atlantis with
|
Latest commit: |
32a50be
|
Status: | ✅ Deploy successful! |
Preview URL: | https://88228c2d.atlantis.pages.dev |
Branch Preview URL: | https://job-122923-job-124608-add-st.atlantis.pages.dev |
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.
Thanks for the excellent contribution! I had some questions and suggestions, let me know if you want to chat through anything.
<Canvas> | ||
<UseStepper /> | ||
</Canvas> |
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 example is great. Consider adding another block here to make the usage example easy to access on this page. Something like:
<Canvas> | |
<UseStepper /> | |
</Canvas> | |
## Example | |
<Canvas> | |
<UseStepper /> | |
</Canvas> | |
```tsx | |
const steps = ["step1", "step2", "step3"] as const; | |
const { currentStep, nextStep, previousStep, isFirst, isLast } = useStepper( | |
steps, | |
{ | |
defaultStep: "step1", // optional | |
}, | |
); | |
return ( | |
<div> | |
{currentStep === "step1" && <div>Step 1</div>} | |
{currentStep === "step2" && <div>Step 2</div>} | |
{currentStep === "step3" && <div>Step 3</div>} | |
<Flex gap="base" direction="row" align="end" template={["grow", "grow"]}> | |
<Button | |
label="Previous" | |
onClick={previousStep} | |
disabled={isFirst} | |
type="primary" | |
/> | |
<Button | |
label="Next" | |
onClick={nextStep} | |
disabled={isLast} | |
type="primary" | |
/> | |
</Flex> | |
</div> | |
); |
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.
Also consider including goToStep
in your example here, as it is a very useful feature.
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.
Good call, I've added another button here that is rendered when we're at the last step, and returns to the beginning of the stepper to demonstrate that functionality
}); | ||
}); | ||
|
||
describe("useStepper implementation", () => { |
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.
These tests are good, but do they add anything that the above approach isn't covering?
Consider eliminating one of the two approaches here unless they are both required.
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.
Good point, its probably a bit redundant to have both 🤔
setCurrentStep(step); | ||
}, | ||
|
||
nextStep: () => { |
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 find nextStep
and previousStep
ambiguous - they seem to be nouns, like currentStep
since they omit their verb.
Consider making these clearer with a noun. One example, consistent with goToStep
could be goToNextStep
and goToPreviousStep
I recognize that that will mean existing usages of this hook will need to be migrated to the new functions but I think it is worth it for clarity in the long run.
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've renamed these properties as you suggested, there are only a couple of existing usages and it should be a fairly easy refactor when we start using the Atlantis component
Motivations
There are a number of places where we have multi-step workflows in JO and JFE, with differing implementations. The aim is to provide a standard implementation for multi-step workflows which can be shared among repositories.
This hook has been vetted in JFE, having been used to build the multi-step setup guide, and will also be used in certain multi-step workflows in JO, such as the self-serve cancellation/hibernation workflow, and upcoming self-serve downgrade process
Changes
Added
useStepper
hook based on the existing implementation in JFEChanged
Deprecated
Removed
Fixed
Security
Testing
A basic example of the stepper hook has been added to storybook and can be used to test the usage of navigating forward/back in a stepper workflow. The checks for first/last step are also being used in the example to enable/disable stepper buttons
Changes can be
tested via Pre-release
In Atlantis we use Github's built in pull request reviews.