-
Notifications
You must be signed in to change notification settings - Fork 17
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
ButtonIcon: Add ledger. #480
Conversation
src/components/Icon/Icon.jsx
Outdated
@@ -1402,11 +1402,12 @@ export const IconWrapper = React.forwardRef( | |||
const defaultIconBgColor = | |||
backgroundColor || getThemeProperty(theme, "icon-background-color"); | |||
const isStringSize = typeof size === "string"; | |||
const vb = type === "ledger" ? "0 0 148 128" : viewBox; |
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.
you should pass width and height instead of hardcoding ?
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.
Is the viewBox not always the same for the same full svg? I don't understand %100 but If we just want the ledger logo, we want this, if we want to change the size we would change the size...
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.
I need to find a way to show this live...
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.
So, can play with that, but view box will just make a bunch of white space you can't control or not show the logo at all. Seems leaving static and adjusting the size is correct.
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.
hrmmm then i think we may need a designer to tweak the svg viewbow to match the default we use for all other svgs
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.
@tiagoalvesdulce i know that you did that in the past pretty straight forward, would like to get
your help/opinion
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.
Yeah, changing the viewbox to our default would also be a good answer. I will try some simple things to see if I can (load and save the svg with gimp/imagemagick).
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.
Last change changes the original to work with the "0 0 450 450" viewbox https://github.com/decred/pi-ui/compare/b00ad6092b255870079cf2cb03175ef3be8412fc..af3dfbe00bc6f7d85a9ad61a6d42641792a0575c
Created by taking the svg from here https://developers.ledger.com/docs/transport/ledger-logos/ and the following transformation:
rsvg-convert ledger-square.svg -a -w 450 --page-width 450 --page-height 450 --top 29 -f svg -o ledger450.svg
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.
awesome!!!!
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.
sorry for the late response. Yeah, I used to use Sketch to adjust the viewbox.
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.
LGTM
It appears I need the ledger tag added there to Button Icons in order to import to decrediton... Also unsure how to use the exported settings. It appears that the view box needs to be set to those values? I am an idiot here though so please help me correct but this is working. @amass01