DOP-4436: Add explicit height/width for leafy-green icons #1042
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mayaraman19
changed the title
mmeigs
reviewed
Mar 20, 2024
src/components/Card/index.js
Outdated
Comment on lines
156
to
162
|
|
||
| const styles = ['landing', 'product-landing'].includes(template) | ||
| ? { dim: dim, style: '' } | ||
| : { dim: dim, style: compactIconStyle }; | ||
|
|
||
| const styling = [ | ||
| cardBaseStyles, |
There was a problem hiding this comment.
This naming (styles and styling) is going to be confusing. I'd say either rename both of them to be more specific to what piece they're styles for. OR You could simply use dim as a variable on it's own within the <img> tag, and then have a one-liner for if the img needs the compactIconStyle
There was a problem hiding this comment.
Also, TOTAL NIT: could you rename dim to be slightly more indicative of what it is? iconDimension or imgDimension or anything to ensure when we look at this in a few months we have a good idea of what it is without searching
mmeigs
approved these changes
Mar 21, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Stories/Links:
DOP-4436
This PR adds explicit height/width attributes to the
<img>tags nested in components/directives (namely cards and permalinks), so that we can improve our performance as gauged by https://pagespeed.web.dev/. I have included various staging links, which you can run through the aforementioned link and compare the results (under "Image elements do not have explicit width and height") to production. I've also included results for running the test for staging links and production links.Staging Environments
docs-landing
homepage staging link (look at both homepage and Migrators, Tools, and Connectors)
production page speed results
staging page speed results
production MTC results
staging MTC results
cloud-docs landing page
staging link
production results
staging results
drivers landing page (
docs-ecosystem)idk why the c++ logo is broken but i swear it works in my local environment 😭
staging link
production results
staging results
404 page
staging link
production results
staging results
Notes:
Although the title of the ticket says "leafy-green icons," I didn't touch the actual
Iconcomponent because 1. I couldn't really find any examples of LeafyGreen Icons in a cursory search of some of the larger properties, 2. the component uses a LeafyGreen icon, not an image, and 3. I figured the impact is likely minimal. Open to any advice, though, since the ticket is slightly ambiguous.README updates