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

fix: Align the delete icon for change mutisig modal #451

Merged
merged 4 commits into from
Dec 5, 2023

Conversation

Lykhoyda
Copy link
Collaborator

@Lykhoyda Lykhoyda commented Dec 4, 2023

closes #194

  • Aligned delete icon
  • Adjust Hover state for the delete button
  • Refactored to styled component

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
  • Looks good for multisig with proxy

@Tbaut
Copy link
Collaborator

Tbaut commented Dec 4, 2023

This looks much better!
I just checked on smaller screens and I think we should reduce the outside margins of the "selected" area because this messes up the design, on iphone 11, 12, 13 etc.

image

@asnaith
Copy link
Contributor

asnaith commented Dec 4, 2023

Looking great except for the small device / margin issue which Thibaut already mentioned.

I'm guessing the screenshot below is also the same / similar issue? Here the last two are not in alignment although they don't break out of the parent (this is the "Pixel 7" layout in dev tools).

CleanShot 2023-12-04 at 11 10 39@2x

I really like the new hover highlighting the entire row, that's a good visual tweak

@Lykhoyda
Copy link
Collaborator Author

Lykhoyda commented Dec 5, 2023

@Tbaut and @asnaith thank you for the feedback, good catch with the mobile version! I updated the code, so please try again. Especially the Pixel 7 layout as I don't have such a multisig with a lot of accounts

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.

Looking great now, thank you!

A couple notes for the PR

  • please remove the checkboxes in the first post if you think they don't apply, so that we don't see them as a "todo" from the implementer that aren't done yet
  • regarding the mobile view, this checkbox is exactly there for the reason that implementers should remember to check some of the smallest recent phones, such as the iphones SE. We of course don't want to support every single mobile phone.

@Lykhoyda Lykhoyda merged commit f905d43 into main Dec 5, 2023
6 checks passed
@Lykhoyda Lykhoyda deleted the lykhoyda/fix_delete_icon_change_multisig branch December 5, 2023 14:45
@asnaith
Copy link
Contributor

asnaith commented Dec 5, 2023

@Tbaut and @asnaith thank you for the feedback, good catch with the mobile version! I updated the code, so please try again. Especially the Pixel 7 layout as I don't have such a multisig with a lot of accounts

@Lykhoyda Yep, I've re-tested with the latest version of code and all looking great now :)

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.

Align the delete icons on "Change Multisig" modal
3 participants