-
Notifications
You must be signed in to change notification settings - Fork 6
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
Rewrite the frontend to React #19
base: master
Are you sure you want to change the base?
Conversation
As the issue mentions current best partices, maybe the react implementation can directly use v2 and implement SIWE like in the other PR. |
SIWE is ideal for the frontend, but I'm not sure if we should also suggest using it in the backend. |
- adds husky for pre-commit hooks - lint-staged for formatting only staged changes # Conflicts: # pnpm-lock.yaml
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.
- When the user rejects the signing the
Something went wrong
blocks any further interaction => I think giving a way to the user to retry would be good - When setting a new message, it also asks for the view call to read the message, but still isn't revealing it and you need to click
tap to reveal
=> it can be confusing for the user why he needs to sign more than a tx when setting a message, I think it would better to have the signed view call only when the user want to reveal the message - (nice to have) I think it would be nice if the account was already connected, the user keeps being connected when refreshing the page, most dapps don't lose the connected account. (is maybe a feature of Rainbowkit etc)
All the points were addressed in e8c62d6 The reason for not using Rainbowkit is simple, as far as I know currently there is no "sapphire wrapper" support for viem-v2, hence no support for signed queries. Should also be migrated in #21 |
- retry on failing message retrieval - set message, does not try to fetch new message - connect wallet on page load
- previous one was outdated
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.
LGTM now.
Only thing I noticed is, that with the current contract in env.development, I could also read the message from an account which wasn't the author. Doing fresh deploy of the contract and using it, it worked then as expected.
Description
Resolves #18