Skip to content
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

Merged
merged 14 commits into from
Apr 2, 2024

Conversation

valefar-on-discord
Copy link
Collaborator

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:

  • Avoid using Material layout components as the issues with Material Grid are concerning: Building v5 and then immediately having to create an alternative "Unstable Grid2" to help with backward compatibility
  • Move away from semantic styling and use a TailwindCSS
  • Strip out prop drilling and use React Context to store long-lasting state
  • Break out each step into a Page and have routing dictate the flow

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

@remyroy remyroy self-requested a review March 26, 2024 15:02
@remyroy
Copy link
Member

remyroy commented Mar 26, 2024

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.

@valefar-on-discord
Copy link
Collaborator Author

Good catch. I accidentally pushed changes to a branch instead of main.

Fixed.

I will test the other two flows this afternoon.

@remyroy
Copy link
Member

remyroy commented Mar 26, 2024

I meant for any PR, it would be better to define the source of the PR as a branch instead of main just to avoid any confusion and let your main be a way to merge from upstream for instance.

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.

@valefar-on-discord
Copy link
Collaborator Author

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
@valefar-on-discord
Copy link
Collaborator Author

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.

Copy link
Member

@remyroy remyroy left a 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 {
Copy link
Member

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

Copy link
Member

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack

@remyroy
Copy link
Member

remyroy commented Mar 28, 2024

When on the using an existing secret recovery phrase and generating existing or new validator keys, Create Keys page include a field which title is not all visible: Amount of Existing (starting inputindex)

field-title

There are many possible solutions to this issue. I'm open to all of them.

@valefar-on-discord
Copy link
Collaborator Author

I fixed the input label issue by making the fields equal width and removing the extranous input that was a mistake.

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!

@remyroy
Copy link
Member

remyroy commented Apr 2, 2024

Everything is good. Thanks for this great effort!

@remyroy remyroy merged commit 22ee337 into stake-house:main Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants