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

Add explicit avatar height/width #63

Merged
merged 2 commits into from
Mar 8, 2021
Merged

Add explicit avatar height/width #63

merged 2 commits into from
Mar 8, 2021

Conversation

squalrus
Copy link
Contributor

@squalrus squalrus commented Mar 2, 2021

Seeing a few issues in PageSpeed Insights related to the avatar image size not being set and content shifting. Thoughts on the proposed changes?

image

image

Copy link
Owner

@vaga vaga left a comment

Choose a reason for hiding this comment

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

If you define the width/height in HTML element, we can remove max-width and max-height in CSS file.
I don't understand why it doesn't take CSS properties. If you have an explanation, let me know.

@vaga vaga added the enhancement New feature or request label Mar 3, 2021
@squalrus
Copy link
Contributor Author

squalrus commented Mar 3, 2021

I don't understand why it doesn't take CSS properties. If you have an explanation, let me know.

I believe a CSS-only solution would be acceptable, but would have to be updated to height and width, rather than max-height and max-width. Currently the text could appear before the image causing the Cumulative Layout Shift to occur when the image "pops" in.

I think the foolproof solution, and part of Google's AMP initiative, is to add the height and width to the images themselves (assuming the size does not change via CSS).

Let me know how you'd like to proceed and I can update the pull request!

@vaga
Copy link
Owner

vaga commented Mar 4, 2021

I prefer to remove max- prefix if it works, my next issue is to rework the header for better mobile integration. (#61)

@squalrus squalrus requested a review from vaga March 7, 2021 22:45
@squalrus
Copy link
Contributor Author

squalrus commented Mar 7, 2021

@vaga updated! Let me know what you think.

@vaga vaga merged commit 3094e3f into vaga:master Mar 8, 2021
@squalrus squalrus deleted the patch-1 branch March 8, 2021 16:20
@squalrus
Copy link
Contributor Author

Hey @vaga! Wanted to follow-up on this after rolling out the latest changes. I'm now seeing 100% for my site and the Cumulative Layout Shift is no longer an issue thanks to the CSS update.

image

Thanks again for the awesome, performant theme!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants