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

Feature/connect auth #341

Merged
merged 2 commits into from
May 30, 2023
Merged

Feature/connect auth #341

merged 2 commits into from
May 30, 2023

Conversation

DemogorGod
Copy link
Contributor

connected auth flow and fixed shifting on modal opening.

@DemogorGod DemogorGod requested a review from ccali11 May 25, 2023 14:15
@ccali11
Copy link
Contributor

ccali11 commented May 30, 2023

First I will review from a UX perspective. I will consolidate my comments into a list and send after the review.
Then I will take a look at the code.

I'd appreciate if @hawyar arbitrates any feedback you disagree with.

Copy link
Contributor

@ccali11 ccali11 left a comment

Choose a reason for hiding this comment

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

User Interface Feedback

Looking great!

User Experience Feedback

  • As a user, I would expect the "Select Provider" modal to completely change to a "Select Address" modal instead of having a dropdown above the providers.
  • As a user, I would like a way of knowing that I'm confirmed signed up or logged in or that my address was added to my user account
  • As a user, I would like a way of knowing if I'm currently logged in or not (this could possibly be solved with however we solve the above point about confirming user successfully signed up, logged in, or added an address to their user account)
  • Small bug where after connecting a first wallet address (say with provider MetaMask), then when I go to select Ledger, for a moment the MetaMask address still shows

Code Feedback

  • Looks like we can remove the feather library from package.json if we're going to use cdn

apps/web/index.html Show resolved Hide resolved
apps/web/src/App.vue Show resolved Hide resolved
apps/web/src/composables/ethers.ts Show resolved Hide resolved
@ccali11 ccali11 self-requested a review May 30, 2023 19:51
@ccali11 ccali11 merged commit 5b429b0 into develop May 30, 2023
@ccali11 ccali11 deleted the feature/connect-auth branch May 30, 2023 19:53
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.

None yet

2 participants