-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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(ui): remove gradients from logo svg components #17214
Conversation
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.
General request. Can we move the styles into an scss
file? Can we also make the classNames a bit more descriptive?
@@ -9,6 +9,7 @@ const GoLogo: SFC = () => { | |||
height="100%" | |||
xmlns="http://www.w3.org/2000/svg" | |||
viewBox="0 0 300 300" | |||
xmlSpace="preserve" |
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.
.js9{fill:url(#SVGID_5_);} | ||
.js10{fill:url(#SVGID_6_);} | ||
.js5{fill:#699F63;} | ||
.js6{fill:#699F63;} |
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.
Can we move these to a stylesheet?
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.
this matches the format of our other logo files. They are autogenerated, so i don't know how useful this would be.
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.
True, but we can also make things a little clearer if we wanted to. It's definitely not a deal breaker but something to keep in mind since we are doing it here:
.js7{fill:#699F63;} | ||
.js8{fill:#699F63;} | ||
.js9{fill:#699F63;} | ||
.js10{fill:#699F63;} | ||
`} | ||
</style> | ||
<rect className="js0" width={300} height={300} /> |
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.
Can we rename these classes to be a bit more descriptive?
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.
this matches the format of our other logo files. They are autogenerated, so i don't know how useful this would be.
Closes #16906
<linearGradient>
within an inline SVG does not work in Safari so long as a<base/>
tag is present in the DOM (which it is)<base />
tag we removed the gradients in favor of flat colorsHere is the working svgs in Safari