-
Notifications
You must be signed in to change notification settings - Fork 432
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
Feature/786 onboarding wizard #880
Feature/786 onboarding wizard #880
Conversation
v3.0.20 release 🚀
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.
Looks great, do you mind fixing the conflicts so we can complete the review 🙏🏻
282c36f
to
d5da8da
Compare
@mlabouardy and @ShubhamPalriwala I have rebased my branch on master and updated the PR, this seems to have handled the merge conflict, kindly review and provide feedback. |
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.
Hey @Kolawole99, just reviewed the PR at the code-level and absolutely amazing work on this 💯! You defined type-casting as well for most of the minute details as well. Hats off 🥇
Just a few small nits here and there,
- For the react components in
dashboard/components/icons
: Can we save them as SVG somewhere in thepublic/assets/img
and import that SVG here. This would improve the code modularity. Do you think this is possible? - For all the pending tasks ie the routes in the backend. input file parsing, can we add a comment line starting with something like:
// TODO: (onboarding-wizard)
so that we have easier visibility on the pending tasks? - For the providers components under the
dashboard/pages/onboarding/<provider>.tsx
, can we move them todashboard/pages/onboarding/providers/<provider>.tsx
? Not sure if it is the best name though, what do you think? - For all the uses of
const provider = 'digitalocean'
, can we have an enum data class/interface which we can use across the project in the future as well? this would avoid string typos/casing errors and more uniform usability across the project?
Let me know if you need any help on the above from my end, would be an honour to help you wherever you face any issues!
I'll now give the feature a try locally 🙌🏼 and see if I can help anywhere in any improvements, which I highly doubt after the sneak peaks I took from the community calls haha, they were top notch 💯
@ShubhamPalriwala and @mlabouardy I have handled all the requested changes for the PR.
|
@ShubhamPalriwala I did not make changes to the Components in the icon folder because there are also other reusable components there that return SVGs, I feel we could leave the icons there since that has been a convention, with just some icons placed in the public folder. |
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.
Hey @Kolawole99, the code now LGTM! 🚀 thanks a ton for this!
Problem
[Provide a brief description of the problem that you are trying to solve with this pull request.]
Solution
[Provide a detailed explanation of the solution you are proposing, including any relevant code snippets or screenshots.]
Changes Made
How to Test
[Provide instructions on how to test the changes you made, including any relevant details like configuration steps or data to be used for testing.]
Screenshots
[Include screenshots, if relevant, to help reviewers understand the changes you made.]
Notes
[Any additional notes or information that you would like to share with the reviewers.]
Checklist
Reviewers
@[username of the reviewer]