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: use default viewbox for custom icons #1031

Merged
merged 7 commits into from
Nov 4, 2022
Merged

fix: use default viewbox for custom icons #1031

merged 7 commits into from
Nov 4, 2022

Conversation

iamacook
Copy link
Member

@iamacook iamacook commented Nov 2, 2022

What it solves

Resolves #1025

How this PR fixes it

All custom SVG icons are now width="24" height="24" viewBox="0 0 24 24" by default and fontSize="small" sets them to 16x16.

Two notable fixes include the rocket icon for execution and (un-)pinning Safe Apps icons.

How to test it

Open the Safe and observe all icons are correctly sized when compared to Figma.

@iamacook iamacook self-assigned this Nov 2, 2022
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Nov 2, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 1c1c972
Status: ✅  Deploy successful!
Preview URL: https://7be8b92f.web-core.pages.dev
Branch Preview URL: https://standarse-icons.web-core.pages.dev

View logs

@github-actions
Copy link

github-actions bot commented Nov 2, 2022

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@usame-algan
Copy link
Member

usame-algan commented Nov 2, 2022

The sizes look much more consistent now 👍

A few things I noticed while testing:

  • Safe Apps icon color is too dark and the link icon should have all circles outlined

Screenshot 2022-11-02 at 17 45 29

  • Owner Settings icons need to be a bit more spaced out

Screenshot 2022-11-02 at 17 46 49

  • Whats new icon

Screenshot 2022-11-02 at 17 47 17

  • The info icon is too dark and should be outlined

Screenshot 2022-11-02 at 17 48 09

  • In dark mode, the icons should have the color #A1A3A7 instead of #636669

Screenshot 2022-11-02 at 17 48 58

  • There are two slightly different versions for the qr, copy and external link icon in Figma. Which ones should we use? cc @liliiaorlenko

Screenshot 2022-11-02 at 17 51 34

Screenshot 2022-11-02 at 17 51 48

@iamacook
Copy link
Member Author

iamacook commented Nov 3, 2022

Thank you for the thorough review. I've ticked all the comments that I've addressed as follows:

  • Safe Apps icon color is too dark and the link icon should have all circles outlined

The share button is now correct and bookmark icon is "border.main" (although the design uses "primary.main") as I agree it looks much more consistent:

image
image

  • Owner Settings icons need to be a bit more spaced out

I've centralised the action spacing to be in EnhancedTable. This means we no longer have to style the row in other places, i.e. in AddressBookTable:

image

  • Whats new icon

This now renders correctly:

image

  • The info icon is too dark and should be outlined

This is now our custom icon (not MUI's) and the correct colour:

image

  • There are two slightly different versions for the qr, copy and external link icon in Figma.

The SidebarHeader icons seem to be a lot bolder that those in our icon collection. I've kept the often used QR, copy and link icons in ./public/images/common and added {{name}}-bold.svg versions in ./public/images/sidebar for now:

image


The following seems to be an inconsistency in the design as all other buttons of a similar action also use "border.main". I think we should keep it as is. cc @liliiaorlenko

  • In dark mode, the icons should have the color #A1A3A7 instead of #636669:
Screenshot 2022-11-02 at 17 48 58

@usame-algan
Copy link
Member

The Tooltip is not being displayed anymore when hovering the info icon next to Safe Nonce in Settings.
Screenshot 2022-11-03 at 11 50 12

The icon colors in dark mode are not #A1A3A7 but rgb(99, 102, 105). It looks like some default fallback of MUI
Screenshot 2022-11-03 at 11 52 43

The new Send/Receive Icons appear a bit larger than other icons. In Figma they are 10px instead of 16px like the rest. We might be able to solve it by scaling them down while still keeping the same viewbox. This would have to be done before exporting though cc @liliiaorlenko
Screenshot 2022-11-03 at 11 52 59

@iamacook
Copy link
Member Author

iamacook commented Nov 3, 2022

The Tooltip is not being displayed anymore when hovering the info icon next to Safe Nonce in Settings.

This is now fixed:

image

The icon colors in dark mode are not #A1A3A7 but rgb(99, 102, 105).

rgb(99, 102, 105) is #636669 which is "border.main". This is the standard colour we use for all basic icons, e.g. the chevrons in the header.

The new Send/Receive Icons appear a bit larger than other icons. In Figma they are 10px instead of 16px like the rest. We might be able to solve it by scaling them down while still keeping the same viewbox.

They appear larger but they are all the same:

image

@usame-algan
Copy link
Member

usame-algan commented Nov 3, 2022

rgb(99, 102, 105) is #636669 which is "border.main". This is the standard colour we use for all basic icons, e.g. the chevrons in the header.

You are right, most icons I can find in Figma have #636669 but there are some with #A1A3A7 and even #B2B5B2 in dark mode. @liliiaorlenko could you clarify which color to go with for all icons or if there is a system behind the different colors?

They appear larger but they are all the same:

What I mean is that the send and receive Icons should be smaller than the other icons. In Figma they are 10x10px instead of 16x16px like the rest of the icons. In order not to implement these exceptions on our side I suggest we export them scaled down to 10x10 but with the same viewbox of 24x24.

@liliiaorlenko
Copy link

So we have to exclude colour #636669, and only use #A1A3A7 in light mode. In dark mode #636669.
As for the sizes I can export all the icons in 16px if needed so we would have only 16px and 24px

@iamacook
Copy link
Member Author

iamacook commented Nov 4, 2022

What I mean is that the send and receive Icons should be smaller than the other icons. In Figma they are 10x10px instead of 16x16px like the rest of the icons.

The icons are now in the correct resolutions:

image

Copy link
Member

@usame-algan usame-algan left a comment

Choose a reason for hiding this comment

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

💯 🚀

@iamacook
Copy link
Member Author

iamacook commented Nov 4, 2022

I'm going to merge this directly as it is purely image-based.

@iamacook iamacook merged commit 44e02ce into dev Nov 4, 2022
@iamacook iamacook deleted the standarse-icons branch November 4, 2022 10:37
@katspaugh katspaugh mentioned this pull request Nov 7, 2022
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.

Some icons are too small
3 participants