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/Enroll - On invite link #261

Merged
merged 3 commits into from
Sep 28, 2023
Merged

Feature/Enroll - On invite link #261

merged 3 commits into from
Sep 28, 2023

Conversation

gguijarro-c-chwy
Copy link
Contributor

image

@gguijarro-c-chwy
Copy link
Contributor Author

Fails
🚫 ❗ PR has more than 500 line changes

Pull Request size seems relatively large. If Pull Request contains multiple changes, split each into separate PR will helps faster, easier review.

Generated by 🚫 dangerJS against 30ef7ca

Copy link
Collaborator

@n13 n13 left a comment

Choose a reason for hiding this comment

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

UI looks great

Code LGTM - my main concern would be interference with other startup / initialization ways

  • Does this work when there's no account - meaning when there's no account, we get this invite link, and process it as before, and auto-join the DAO as before. And this popup does then not show

  • Can it interfere with other deep link actions - esr signing requests, push notifications?

  • Is it cleared (I guess that yes it is)

Yeah so I guess I will need to test this with no account and with account and when both work it's good.

@n13
Copy link
Collaborator

n13 commented Sep 27, 2023

One change request - parse "daoName" field for the dao name, instead of "dao". "dao" is the id of the dao ... daoName is a string with the full name.

Test data see here:

#220

@n13
Copy link
Collaborator

n13 commented Sep 28, 2023

DAO name in another PR

@n13 n13 merged commit 8babc66 into main Sep 28, 2023
1 of 3 checks passed
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.

2 participants