-
Notifications
You must be signed in to change notification settings - Fork 20
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
Link to specific accounts from the address bar #428
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.
Really fantastic to deliver this with the supporting UI tests! Thank you for capturing all of those different scenarios
I think one area to improve is the amount of info and explanations when users land on the error page. They may feel lost and the "reset" button may feel weird. Open to suggestions (maybe for a follow-up pr). We should probably have a "why do I see this" accordion or something explaining that they landed here with an account selected, that they don't have neither in their Multisig list, nor as watched accounts. And then we can do some check and suggest to connect their accounts if not done or watch the account if not done yet. |
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.
Looks good! Great feature!
There is a small suggestion about the button name when the account is not found and typo to fix. The Transaction test in Cypress will not pass due to Roccoco, so needs to be skiped for now (known issue)
closes #266
This was such a tricky change, that I thought it should go with a full suite of tests.
Now because our cypress extension plugin didn't actually support what I needed, I had to make it evolve, and allow users to connect an extension, then refresh the page without the need to connect again.
This made the scope of this PR explode.. but it was only in order to actually push many many tests. So I hope it alleviates the annoyance.
Note that there's some refactoring to do to make the extension plugin easier to use. I'll do this in another PR see #433
Submission checklist:
Layout
Compatibility