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.

Sign up for GitHub

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

Jump to bottom

Image Compare block: add AMP support using amp-image-slider #15753

Merged
merged 5 commits into from
May 14, 2020
Merged

Image Compare block: add AMP support using amp-image-slider #15753

merged 5 commits into from
May 14, 2020

Conversation

mkaz
Copy link
Contributor

@mkaz mkaz commented May 11, 2020 •
edited
Loading

Fixes #15653

Changes proposed in this Pull Request:

Testing instructions:

  • This only applies to render in AMP, no changes to non-AMP enviornments
  • Install AMP plugin: https://wordpress.org/plugins/amp/
  • Activate and Enable Standard AMP so all pages are rendered as AMP pages (this makes testing easier)
  • Create a new post (or view existing) with an Image Compare block
  • You should see a side-by-side image comparison, it should be the AMP component that looks a bit different than the non-AMP version but should work.

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.

@mkaz mkaz added [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Block] Image Compare labels May 11, 2020
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello mkaz! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D43270-code before merging this PR. Thank you!
This revision will be updated with each commit to this PR

@jetpackbot
Copy link

jetpackbot commented May 11, 2020 •
edited
Loading

Warnings
⚠️

pre-commit hook was skipped for one or more commits
⚠️ "Proposed changelog entry" is missing for this PR. Please include any meaningful changes

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 199648f

@jeherve jeherve changed the title Add AMP support using amp-image-slider Image Compare block: add AMP support using amp-image-slider May 12, 2020
@jeherve jeherve added the AMP label May 12, 2020
@jeherve jeherve added this to the 8.6 milestone May 12, 2020
@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels May 12, 2020
@mkaz mkaz added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels May 13, 2020
mkaz added a commit that referenced this pull request May 13, 2020
@mkaz
Refactor block attributes
b9bfaff 
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
@mkaz mkaz mentioned this pull request May 13, 2020
Merged
jeherve
jeherve requested changes May 14, 2020
View reviewed changes
Copy link
Member

@jeherve jeherve left a 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.

extensions/blocks/image-compare/image-compare.php Outdated Show resolved Hide resolved
@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels May 14, 2020
@mkaz mkaz added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels May 14, 2020
jeherve
jeherve previously approved these changes May 14, 2020
View reviewed changes
@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels May 14, 2020
mkaz added a commit that referenced this pull request May 14, 2020
@mkaz
Refactor block attributes (#15781)
abe0e20 
* 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
mkaz added 3 commits May 14, 2020 10:34
@mkaz
[not verified] Add AMP support using amp-image-slider
d6cc164
@mkaz
[not verified] Add esc_attr() to escape attributes
aa72663
@mkaz
[not verified] Remove source from attributes
3b5ea81 
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.
mkaz and others added 2 commits May 14, 2020 10:36
@mkaz @jeherve
[not verified] Update extensions/blocks/image-compare/image-compare.php
597566f 
Co-authored-by: Jeremy Herve <jeremy@jeremy.hu>
@mkaz
Use new image object attribute
199648f 
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.
@mkaz mkaz added the [Status] Needs Review To request a review from Crew. Label will be renamed soon. label May 14, 2020
@mkaz
Copy link
Contributor Author

mkaz commented May 14, 2020

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.

@jeherve jeherve removed the [Status] Needs Review To request a review from Crew. Label will be renamed soon. label May 14, 2020
@mkaz mkaz merged commit 0d378d4 into master May 14, 2020
@mkaz mkaz deleted the add/15653-imgcmp-amp branch May 14, 2020 21:46
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels May 14, 2020
@mkaz
Copy link
Contributor Author

mkaz commented May 14, 2020

r207508-wpcom

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeherve jeherve jeherve approved these changes

Assignees

@mkaz mkaz

Labels
AMP [Block] Image Compare Touches WP.com Files
Projects
None yet
Milestone
8.6
Development

Successfully merging this pull request may close these issues.

Image Compare Block: Add AMP support
4 participants
@mkaz @matticbot @jetpackbot @jeherve