-
-
Notifications
You must be signed in to change notification settings - Fork 47
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
Material v4-v5 upgrade and wagyu refactor #189
Conversation
Wagyu refactor
Is your main branch from https://github.com/valefar-on-discord/wagyu-key-gen the correct one to review? It would be better to put everything in a specific branch for any change or fix simply to seperate everything. |
Good catch. I accidentally pushed changes to a branch instead of main. Fixed. I will test the other two flows this afternoon. |
I meant for any PR, it would be better to define the source of the PR as a branch instead of This is fine if you want to keep this PR this way and avoid having to redo the PR/branches in your local repo. I'll wait until you complete more tests until the full review. |
Ah I gotcha. I'll keep that in mind for the future. Hopefully it won't cause issues this time around. |
Highligh BTEC change file when opening finder on finish page Fixing styling issues with OnlineDetector
I'm comfortable to hand this off for full review. I'm getting a little tunnel vision at this point and I think a second pair of eyes will help but I did thoroughly test the three flows of creating keys, recreating keys, and generating the BTEC change. I verified the UX against v1.10 and compared the output of each. This is not a pixel-perfect update so there will be some changes here and there but happy to resolve any discrepancies. |
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.
Some initials things to check.
|
||
import WagyuModal from "./WagyuModal"; | ||
|
||
interface OlineWarningModalParams { |
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.
This interface should be renamed from OlineWarningModalParams
to OnlineWarningModalParams
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 references to this interface should also be renamed to complete the refactor.
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.
ack
inputs be equal width
I fixed the input label issue by making the fields equal width and removing the extranous I went through each file with a spell checker as well as clearly it isn't my speciality :) Thanks for looking at this, really appreciate it! |
Everything is good. Thanks for this great effort! |
Upon attempting to upgrade from Material v4 to v5, I ran into a multitude of layout and styling issues due to the abundant usage of Material Grid containers. This seems to be a common problem when wrapping Grid container in Grid container. The recommended approach: "Don't wrap grid containers". I decided, hey, why not redo it a bit.
As with all things, I went overboard.
Here are some of the design choices I made during this refactor:
The general idea is each flow has four steps, each step has a unique route that will lead to a Page where some pages are reused. Each flow will be wrapped in a React context that is used to store the inputs the user provides and ultimately is used to perform the action. This drastically reduces the complexity of the previous version.
Definitely start with
App.tsx
and go from there.There are still a few small improvements I can make and I need to do a some thorough testing but wanted to provide a 99% done version.
Sorry in advance