-
Notifications
You must be signed in to change notification settings - Fork 799
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
Image Compare block: add AMP support using amp-image-slider #15753
Conversation
Caution: This PR has changes that must be merged to WordPress.com |
This is an automated check which relies on |
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 works well in my tests. I would have one last comment, below.
* Refactor block attributes Due to chagnes for AMP support in #15753 we are changing the attributes to not be pulled from source. This allows switching the images to objects and simplifies the code. Additionally, adds width/height attributes and uses the photonizeImageProps utility to set a photon URL. Fixes: #15741 * Photonize before image too
In order to help AMP and server-side rendering, removing the source will make them available to PHP side to output the proper AMP markup.
Co-authored-by: Jeremy Herve <jeremy@jeremy.hu>
Switches the AMP usage to the new image object introduced in #15781 and adds the width/height that fixes an aspect ratio issue for AMP.
14b0d66
to
199648f
Compare
Rebased with changes from #15781 A minor addition setting default: horizontal for orientation attribute, the block would invalidate if you toggled from above-below and then back to side-by-side and then saved the block. Setting a default ensures the orientation value is always there. |
r207508-wpcom |
Fixes #15653
Changes proposed in this Pull Request:
Testing instructions:
Note: the side-by-side and above-below settings will not alter the viewing (it will always be side-by-side) when viewed in AMP context. The AMP component needs to add the additional orientation support, it is on their roadmap.
I left this as-is instead of disabling the option, because the AMP plugin can also be configured to serve regular and AMP content from the same source (determined by URL). So checking for plugin and disabling would remove the feature for non-AMP content.