stake-house / wagyu-key-gen

GNU General Public License v3.0
61 stars 44 forks source link

Material v4-v5 upgrade and wagyu refactor #189

Closed valefar-on-discord closed 8 months ago

valefar-on-discord commented 8 months ago

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

remyroy commented 8 months ago

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 commented 8 months ago

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

Fixed.

I will test the other two flows this afternoon.

remyroy commented 8 months ago

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 commented 8 months ago

Ah I gotcha. I'll keep that in mind for the future. Hopefully it won't cause issues this time around.

valefar-on-discord commented 8 months ago

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.

remyroy commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

Everything is good. Thanks for this great effort!