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

Rewrite the frontend to React #19

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Rewrite the frontend to React #19

wants to merge 9 commits into from

Conversation

lubej
Copy link
Contributor

@lubej lubej commented Oct 21, 2024

Description

  • changed frontend framework from Vue to React

Resolves #18

@rube-de
Copy link

rube-de commented Oct 25, 2024

As the issue mentions current best partices, maybe the react implementation can directly use v2 and implement SIWE like in the other PR.

@matevz
Copy link
Member

matevz commented Oct 25, 2024

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
@lubej lubej marked this pull request as ready for review November 5, 2024 10:39
@rube-de rube-de self-requested a review November 5, 2024 11:29
Copy link

@rube-de rube-de left a 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)

backend/hardhat.config.ts Outdated Show resolved Hide resolved
@lubej lubej self-assigned this Nov 12, 2024
@lubej
Copy link
Contributor Author

lubej commented Nov 12, 2024

  • 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
Copy link

@rube-de rube-de left a 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.

@lubej
Copy link
Contributor Author

lubej commented Dec 9, 2024

@matevz can we merge this PR? And hold off with #16, or would you rather merge #16 first.

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.

Rewrite the frontend to React
3 participants