Skip to content

Ensure site icon in legacy template is identified as a hero image #6403

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

Merged
merged 3 commits into from
Jun 22, 2021

Conversation

westonruter
Copy link
Member

Summary

Discovered when testing #6402.

  • Add data-hero-candidate to .amp-wp-site-icon.
  • Let .amp-wp-site-icon be img in template so it can convert to amp-img.
  • Add alt attribute to .amp-wp-site-icon.

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).

@github-actions
Copy link
Contributor

Plugin builds for 6d69152 are ready 🛎️!

@westonruter westonruter merged commit 971c5b3 into develop Jun 22, 2021
@westonruter westonruter deleted the update/site-icon-hero branch June 22, 2021 00:03
@westonruter westonruter self-assigned this Jun 30, 2021
@westonruter
Copy link
Member Author

QA Passed

I added a Site Icon and changed the template mode to Reader with the legacy theme:

image

The logo was rendered as a hero:

<amp-img src="https://wordpress-stable.lndo.site/wp-content/uploads/2021/04/cropped-amp-wp-logo-32x32.png" width="32" height="32" class="amp-wp-site-icon amp-wp-enforced-sizes i-amphtml-layout-intrinsic i-amphtml-layout-size-defined" data-hero-candidate="" alt="Site icon" layout="intrinsic" data-hero i-amphtml-ssr i-amphtml-layout="intrinsic">
	<i-amphtml-sizer class="i-amphtml-sizer">
		<img alt="" aria-hidden="true" class="i-amphtml-intrinsic-sizer" role="presentation" src="data:image/svg+xml;base64,PHN2ZyBoZWlnaHQ9IjMyIiB3aWR0aD0iMzIiIHhtbG5zPSJodHRwOi8vd3d3LnczLm9yZy8yMDAwL3N2ZyIgdmVyc2lvbj0iMS4xIi8+">
	</i-amphtml-sizer>
	<img class="i-amphtml-fill-content i-amphtml-replaced-content" decoding="async" alt="Site icon" src="https://wordpress-stable.lndo.site/wp-content/uploads/2021/04/cropped-amp-wp-logo-32x32.png">
</amp-img>

@westonruter
Copy link
Member Author

In testing this, I did find that the hero image failed to get identified for an image block which appears at the beginning of the content in the legacy Reader templates. Fix submitted in #6440.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants