-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add 3Box profile integration to the Snapshot app #172
Conversation
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.
Thank you for your contribution @ChaituVR. I've added some suggestions.
Co-authored-by: Fabien <bonustrack@users.noreply.github.com>
Co-authored-by: Fabien <bonustrack@users.noreply.github.com>
Co-authored-by: Fabien <bonustrack@users.noreply.github.com>
Thanks for the review @bonustrack Added those suggested changes, also added |
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.
The size of the app bundle has been increased by x4 with the adding of 3box have a look here:
Without 3Box: https://snapshot.page/report.html
With 3Box: https://bafybeidq45eittd3vfbbydxd3fcwfwe47xvx3p5xbe7gimyztve44totx4.on.fleek.co/report.html
Is there a way to import only what we need from the dependency? I believe what we need is just to query the 3Box API endpoint, we don't need all the advanced functions.
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.
Ok last changes,
- Can you create a file helpers/3box.ts with a method
getProfiles
and use this method on app.ts and web3.ts, that will reduce duplicate code and make the function more accessible, you don't need to have 2 sub request on for voterProfiles and authorProfile, you could just request profiles of an array of addresses that include both author and voters addresses. - Can you resolve conflicts and and run the "lint" script?
This comment has been minimized.
This comment has been minimized.
Sorry i found 2 more issues: 1: When i connect my wallet here https://bafybeidqtbuxmtfemil2qzio3wf5zlonbcerbv2f2t73s3646yuhue7kim.on.fleek.co then refresh the page, the wallet should be reconnected again and there should be a loading indicator in top nav like this: 2: The size of the dependency 3box is too heavy even using only |
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.
Great work @ChaituVR! The PR is good to be merged
Changes proposed in this pull request:
1. Add profile in the top nav
2. Add profile and button in account modal
If the User already have an account in 3Box, providing a link to edit
if the User is not having an account in 3Box, providing a like to 3Box,
3. Add names and avatars of the voters
URLs to test:
http://localhost:8080/#/balancer/proposal/QmQaxYxNp2Vq9PqW7Xc1Acnup7wC2Yrhg3UiYi9DYBFrPZ
More Votes: http://localhost:8080/#/yearn/proposal/QmUAUtyK7FiEAKF3FCeniWsvSiVxT2J6DESVcBKZYnPrCi
4. Add name and avatar of the proposal author
Inside Proposal: (http://localhost:8080/#/balancer/proposal/QmQaxYxNp2Vq9PqW7Xc1Acnup7wC2Yrhg3UiYi9DYBFrPZ)
5. Add profile in user modal