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

Refactor block attributes #15781

Merged
merged 2 commits into from
May 14, 2020
Merged

Refactor block attributes #15781

merged 2 commits into from
May 14, 2020

Conversation

mkaz
Copy link
Contributor

@mkaz mkaz commented May 13, 2020

Due to changes 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

Changes proposed in this Pull Request:

  • Switch attributes to remove source
  • Switch image attributes to be objects
  • Add width/height attributes to images
  • Use photonizeImageProps to set src

Testing instructions:

  • Due to refactoring attributes, any old Image Compare blocks will invalidate
  • Create a new Image Compare block
  • Confirm block works as expected on edit/view/re-edit

Note: after this change goes in, the AMP part will need to be updated to include width/height that will also fix the aspect ratio issue.

Proposed changelog entry for your changes:

  • For this PR, not necessary because the feature it is modifying is all new. So rolls up to the top entry which should be something like: "New Image Compare block"

@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 self-assigned this May 13, 2020
@mkaz mkaz added [Block] Image Compare [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels May 13, 2020
@mkaz mkaz requested a review from jeherve May 13, 2020 16:31
@jetpackbot
Copy link

jetpackbot commented May 13, 2020 •
edited
Loading

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: June 2, 2020.
Scheduled code freeze: May 26, 2020

Generated by 🚫 dangerJS against 340c72f

@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 D43428-code before merging this PR. Thank you!
This revision will be updated with each commit to this PR

1 similar comment
@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 D43428-code before merging this PR. Thank you!
This revision will be updated with each commit to this PR

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.

I like this! Can we do the same for both images?

extensions/blocks/image-compare/edit.js 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
@jeherve jeherve added this to the 8.6 milestone 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 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 mkaz merged commit abe0e20 into master May 14, 2020
@mkaz mkaz deleted the update/15741-imgcmp-attribs branch May 14, 2020 16:34
@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

r207483-wpcom

mkaz added a commit that referenced this pull request May 14, 2020
@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 mentioned this pull request May 14, 2020
Merged
mkaz added a commit that referenced this pull request May 14, 2020
@mkaz @jeherve
Image Compare block: add AMP support using amp-image-slider (#15753)
0d378d4 
* [not verified] Add AMP support using amp-image-slider

* [not verified] Add esc_attr() to escape attributes

* [not verified] Remove source from attributes

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.

* [not verified] Update extensions/blocks/image-compare/image-compare.php

Co-authored-by: Jeremy Herve <jeremy@jeremy.hu>

* Use new image object attribute

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.

Co-authored-by: Jeremy Herve <jeremy@jeremy.hu>
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
[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 extra image attributes
4 participants
@mkaz @jetpackbot @matticbot @jeherve