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

Improve author avatar resolution and fix a11y issues in legacy theme #7492

Merged
merged 3 commits into from
Mar 20, 2023

Conversation

thelovekesh
Copy link
Collaborator

@thelovekesh thelovekesh commented Mar 20, 2023

Summary

  • Update gravatar image resolution to use the default size(96).
  • Update default header color from #0a89c0 to #0A5F85 for better a11y.

Fixes #7491

Checklist

  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@thelovekesh thelovekesh self-assigned this Mar 20, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Mar 20, 2023

Plugin builds for 28bdcc3 are ready 🛎️!

@milindmore22
Copy link
Collaborator

The Default size of gravatar is 96 if we go with default settings it will slightly increase the size of the image and the response time.

Image Size 36 96(default)
Size (kB) 2.7kB 12.9kB
Response time 7ms 27ms

Some images recommend different sizes like 48, also not sure how it will behave with low res images, in my findings using size 36 on the anonymous image also worked for me.

@thelovekesh
Copy link
Collaborator Author

thelovekesh commented Mar 20, 2023

Lighthouse on Chrome 111 displays a failed audit for the gravatar size below 48, although PageSpeed Insights and Lighthouse 10 do not provide the same results and pass the audits with a size of 36.

So I think we can keep the default size as 36 in legacy themes? Or we can keep it 48, exactly double of height/width we have specified for Gravatars.

@thelovekesh thelovekesh added this to the v2.4.1 milestone Mar 20, 2023
@@ -23,7 +23,7 @@
<?php if ( $post_author ) : ?>
<div class="amp-wp-meta amp-wp-byline">
<?php if ( function_exists( 'get_avatar_url' ) ) : ?>
<amp-img src="<?php echo esc_url( get_avatar_url( $post_author->user_email, [ 'size' => 24 ] ) ); ?>" alt="<?php echo esc_attr( $post_author->display_name ); ?>" width="24" height="24" layout="fixed"></amp-img>
<amp-img src="<?php echo esc_url( get_avatar_url( $post_author->user_email ) ); ?>" alt="<?php echo esc_attr( $post_author->display_name ); ?>" width="24" height="24" layout="fixed"></amp-img>
Copy link
Member

Choose a reason for hiding this comment

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

I suppose it is overkill to add srcset to pick the right resolution?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed, I believe. Here browser needs an image with the proper resolution for the specified "24x24" size ratio which is going to be same on all devices.

Copy link
Member

Choose a reason for hiding this comment

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

WordPress core isn't adding srcset so probably not.

Copy link
Member

Choose a reason for hiding this comment

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

But why 96? If we're targeting 3x pixel density, then shouldn't it be 72 (3*24)?

Indeed, I believe. Here browser needs an image with the proper resolution for the specified "24x24" size ratio which is going to be same on all devices.

I don't think that's right, as some devices have 3x pixel density (e.g. "Retina") others have 2x density, and others have 1x.

Copy link
Member

Choose a reason for hiding this comment

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

For example:

<img src="flower.jpg"
  srcset="flower-1x.jpg 1x,
          flower-2x.jpg 2x,
          flower-3x.jpg 3x">

cf. https://web.dev/codelab-density-descriptors/

Copy link
Member

@westonruter westonruter Mar 20, 2023

Choose a reason for hiding this comment

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

So I think the very best here would be something like:

<amp-img
  src="<?php echo esc_url( get_avatar_url( $post_author->user_email, [ 'size' => 72 ] ) ); ?>" 
  srcset="
    <?php echo esc_url( get_avatar_url( $post_author->user_email, [ 'size' => 24 ] ) ); ?> 1x,
    <?php echo esc_url( get_avatar_url( $post_author->user_email, [ 'size' => 48 ] ) ); ?> 2x,
    <?php echo esc_url( get_avatar_url( $post_author->user_email, [ 'size' => 72 ] ) ); ?> 3x
  "
  alt="<?php echo esc_attr( $post_author->display_name ); ?>" width="24" height="24" layout="fixed"
></amp-img>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

https://web.dev/codelab-density-descriptors/

Thanks for the link. Density descriptors were unknown to me.

Copy link
Member

Choose a reason for hiding this comment

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

I guess I didn't share the best link. Here's a better more general one: https://web.dev/learn/design/responsive-images/#pixel-density-descriptor

@westonruter westonruter modified the milestone: v2.4.1 Mar 20, 2023
@westonruter
Copy link
Member

@milindmore22 How are these changes now looking for you, re: #7492 (comment) ?

@westonruter westonruter merged commit 0a44cea into develop Mar 20, 2023
@westonruter westonruter deleted the fix/legacy-theme-a11y branch March 20, 2023 18:05
@westonruter westonruter changed the title Fix a11y issues in legacy theme Improve avatar resolution and fix a11y issues in legacy theme Mar 20, 2023
@westonruter westonruter changed the title Improve avatar resolution and fix a11y issues in legacy theme Improve author avatar resolution and fix a11y issues in legacy theme Mar 20, 2023
@milindmore22
Copy link
Collaborator

This is a good approach, works as expected

@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Mar 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelogged Whether the issue/PR has been added to release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improvements in Legacy Theme
3 participants