-
Notifications
You must be signed in to change notification settings - Fork 297
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
Enhancement/2468 svg replacing images #2834
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.
This looks good, just one little tweak from me but it's minor enough I'll just commit it before merge review rather than do another round of code review 🙂
assets/js/components/legacy-notifications/dashboard-modules-alerts.js
Outdated
Show resolved
Hide resolved
@googlebot I consent. |
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.
Thanks @ivankruchkoff, this is looking good, just a few things to address. Most importantly is that there shouldn't be any substantial visual changes. Our VRT threshold for failure is zero so it's okay if there are microscopic changes in the image due to differences between img/svg but they should be visually equivalent.
assets/js/components/legacy-notifications/dashboard-modules-alerts.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Evan Mattson <[email protected]>
Co-authored-by: Evan Mattson <[email protected]>
Co-authored-by: Evan Mattson <[email protected]>
Co-authored-by: Evan Mattson <[email protected]>
Co-authored-by: Evan Mattson <[email protected]>
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.
@ivankruchkoff this is looking really close, just a few tweaks left I think.
One comment about the added dynamic screen-based fill and added css rule. I still have a question here about the big change in lock file size as well.
I noticed the VRT images look nearly identical now for small and large sizes, however there was a substantial difference still with the medium size:
svg { | ||
width: 200px; | ||
} |
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.
Looks like we might need to add some responsive styles here or make it relative so that it naturally fills the grid cell width? I'm guessing the previous images would have scaled as well.
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 is purely for the rocket image which was 200px x 167px, and never scaled, see the comparison images. Further confirmation is that the addition of the rule brings no additional VRT test failures, so if this rule has an unintended side effect, it also means we are missing some storybook tests.
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.
@aaemnnosttv updated the rule in 6f76bed
Updated the image in cbeb8a1
assets/sass/components/global/_googlesitekit-publisher-wins.scss
Outdated
Show resolved
Hide resolved
Co-authored-by: Evan Mattson <[email protected]>
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 is super close. The main thing left to address is a discrepancy in the size of the icon which is now slightly larger due to being an svg background image rather than an image.
We need to preserve the 16x16 dimensions of the icon. I found this was possible by applying a background-size: 16px 16px
rule to it with CSS but maybe there's a better way?
So I think WP icons are made to trigger people with OCD. I added some padding to the svg itself, updated it in 574bb2d and then took a screenshot and cropped it to see if the dimensions fit.
For comparison, the old svg icon approach:
Comparing the before/after, the before aligns almost with the dashboard icon, the after aligns with the media library, if we want the sizes to be identically, let me know and I'll update it again. |
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.
Thanks @ivankruchkoff. I'm less worried about the icon matching other WP icons and that the size matches the image we had before.
It looks like it's just a tiny bit larger that it was before:
I tried fiddling with it a little and found that it seems to line up exactly when the svg dimensions match the viewbox
using the original svg on the issue.
I'll push this up so we can put this one to rest 😄
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
@googlebot I consent. |
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! Thanks @ivankruchkoff 👍
Summary
Addresses issue #2468
Relevant technical choices
Checklist