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.

Sign up for GitHub

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

Jump to bottom

update: display the type of proxy on multisig view #490

Merged
merged 7 commits into from
Jan 29, 2024
Merged

update: display the type of proxy on multisig view #490

merged 7 commits into from
Jan 29, 2024

Conversation

bolajahmad
Copy link
Contributor

closes #79
Show the Proxy Type

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: Hide Proxy type when Multi only
  • Looks good for solo multisig
  • Looks good for multisig with proxy

@CLAassistant
Copy link

CLAassistant commented Jan 26, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

Tbaut
Tbaut reviewed Jan 26, 2024
View reviewed changes
Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

Thank you so much for the PR. I left a couple comments

packages/ui/src/pages/Home/MultisigView.tsx Outdated Show resolved Hide resolved
packages/ui/src/utils/proxyTypeRendererUtils.ts Outdated Show resolved Hide resolved
packages/ui/cypress/support/page-objects/multisigPage.ts Show resolved Hide resolved
@Tbaut
Copy link
Collaborator

Tbaut commented Jan 29, 2024

Can you also sign the CLA as per #490 (comment)

@bolajahmad
Copy link
Contributor Author

Can you also sign the CLA as per #490 (comment)

I signed the CLA already, and I also updated the PR

@Tbaut Tbaut mentioned this pull request Jan 29, 2024
Open
Tbaut
Tbaut reviewed Jan 29, 2024
View reviewed changes
Copy link
Collaborator

@Tbaut Tbaut left a comment •
edited
Loading

Choose a reason for hiding this comment

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

It's looking great and working well. Tests are passing locally, I created an issue to add the tests as per our comments it's here #494 . I've left a couple small nit that would be great to change, we can merge right after.

packages/ui/src/pages/Home/MultisigView.tsx Outdated Show resolved Hide resolved
packages/ui/src/utils/stringUtils.ts Outdated Show resolved Hide resolved
packages/ui/src/pages/Home/MultisigView.tsx Outdated Show resolved Hide resolved
packages/ui/cypress/support/page-objects/multisigPage.ts Show resolved Hide resolved
Tbaut
Tbaut approved these changes Jan 29, 2024
View reviewed changes
Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

Thank you so much 🙏 🚀

@Tbaut Tbaut merged commit 1370806 into ChainSafe:main Jan 29, 2024
3 of 5 checks passed
@asnaith
Copy link
Member

asnaith commented Jan 29, 2024

Yeah! Thank you for this contribution @bolajahmad :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asnaith asnaith asnaith left review comments

@Tbaut Tbaut Tbaut approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Show the type of each proxy
4 participants
@bolajahmad @CLAassistant @Tbaut @asnaith