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

Link to specific accounts from the address bar #428

Merged
merged 20 commits into from
Nov 9, 2023
Merged

Link to specific accounts from the address bar #428

merged 20 commits into from
Nov 9, 2023

Conversation

Tbaut
Copy link
Collaborator

@Tbaut Tbaut commented Oct 30, 2023

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

  • Change inspected in the desktop web ui
  • Change inspected in the mobile web ui

Compatibility

  • Functionality of change validated with a connected account with multisig
  • Applicable elements hidden / disabled for watched multisigs / pure
  • Looks good for solo multisig
  • Looks good for multisig with proxy

@Tbaut Tbaut changed the title Deep link in the url Link to specific accounts from in the url Oct 30, 2023
@Tbaut Tbaut changed the title Link to specific accounts from in the url Link to specific accounts from the address bar Nov 2, 2023
@github-actions github-actions bot requested a deployment to multix (Preview) November 2, 2023 21:38 Abandoned
Copy link
Member

@asnaith asnaith left a 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

@Tbaut
Copy link
Collaborator Author

Tbaut commented Nov 7, 2023

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.

Copy link
Collaborator

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

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.

Be able to link to a multisig/pure proxy
3 participants