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

Photon: Ignore data-width and data-height attributes #13961

Merged
merged 4 commits into from
Nov 25, 2019

Conversation

mmtr
Copy link
Member

@mmtr mmtr commented Nov 5, 2019

Fixes #Automattic/wp-calypso#37193

Changes proposed in this Pull Request:

Ensures that only width and height attributes are taken into account by Photon when resizing an image.

Previously it was targeting any *width/*height attribute (such as data-weight and data-height) that are not intended for setting an image size (see Automattic/wp-calypso#37193 (comment)).

Is this a new feature or does it add/remove features to an existing part of Jetpack?

Bugfix

Testing instructions:

  • Enable Photon:

Jetpack ‹ WP Engine Test Site — WordPress

  • Create a post and add an image using HTML like this:
<img src="https://my-domain.com/wp-content/uploads/2019/11/image.png" data-width="100px" />
  • Publish the post.
  • View the post on the front end.
  • Make sure the images has not been resize to 100px.
  • Change the data-width attribute to width and update the post:
<img src="https://my-domain.com/wp-content/uploads/2019/11/image.png" width="100px" />
  • Make sure the image has been resized to 100px.
  • Try to play around with the quotes and spaces and make sure the image is still resized:
<!-- No whitespace -->
<img src="https://my-domain.com/wp-content/uploads/2019/11/image.png"width="100px" />
<!-- Single quotes -->
<img src='https://my-domain.com/wp-content/uploads/2019/11/image.png' width="100px" />
<!-- No whitespace and single quotes -->
<img src='https://my-domain.com/wp-content/uploads/2019/11/image.png'width="100px" />

Proposed changelog entry for your changes:

Photon: Ignore data-width and data-height attributes

@mmtr mmtr added the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Nov 5, 2019
@mmtr mmtr requested review from a team November 5, 2019 13:27
@mmtr mmtr self-assigned this Nov 5, 2019
@jetpackbot
Copy link

jetpackbot commented Nov 5, 2019

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: December 3, 2019.
Scheduled code freeze: November 26, 2019

Generated by 🚫 dangerJS against 87dc0f9

@jeherve jeherve added [Status] Needs Testing We need to add this change to the testing call for this month's release [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Photon aka "Image CDN". Feature developed in the Image CDN package and shipped in multiple plugins labels Nov 5, 2019
@jeherve jeherve added this to the 8.0 milestone Nov 5, 2019
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.

Do you think we could add some unit tests to tests/php/test_class.jetpack_photon.php to cover this new use-case?

@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! [Tests] Needs Tests Some Unit Tests would be really useful to include with this PR. and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Nov 5, 2019
class.photon.php Show resolved Hide resolved
gwwar
gwwar previously approved these changes Nov 6, 2019
Copy link
Contributor

@gwwar gwwar left a comment

Choose a reason for hiding this comment

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

Thanks for digging into this @mmtr . I'm giving tentative approval, but would like two changes if possible:

  • As @jeherve noted, it would be great to add additional unit tests for the filter so we can document the case, and quickly check data assumptions
  • See if we can simplify the regex

Before:
Screen Shot 2019-11-06 at 10 46 33 AM

After:
Screen Shot 2019-11-06 at 10 46 06 AM

@gwwar
Copy link
Contributor

gwwar commented Nov 6, 2019

Taking a second look at the screenshot we may also want to look into that resize query param to see if that value is expected.

@mmtr
Copy link
Member Author

mmtr commented Nov 19, 2019

Taking a second look at the screenshot we may also want to look into that resize query param to see if that value is expected.

Seems to be expected when srcset is present:

$args['resize'] = $width . ',' . $height;

@mmtr
Copy link
Member Author

mmtr commented Nov 19, 2019

Do you think we could add some unit tests to tests/php/test_class.jetpack_photon.php to cover this new use-case?

Done in 87dc0f9. Sorry for the delay!

@mmtr mmtr added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Status] Needs Testing We need to add this change to the testing call for this month's release and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! [Status] Needs Testing We need to add this change to the testing call for this month's release [Tests] Needs Tests Some Unit Tests would be really useful to include with this PR. labels Nov 19, 2019
@gwwar
Copy link
Contributor

gwwar commented Nov 23, 2019

This one is ready for another look @jeherve let us know if we need additional 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 fellow Jetpack developers. Label will be renamed soon. [Status] Needs Testing We need to add this change to the testing call for this month's release labels Nov 25, 2019
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 looks good to me and tests well. Merging.

@jeherve jeherve merged commit d38a655 into master Nov 25, 2019
@jeherve jeherve deleted the fix/photon-data-size-attrs branch November 25, 2019 12:15
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Nov 25, 2019
jeherve added a commit that referenced this pull request Nov 25, 2019
jeherve added a commit that referenced this pull request Nov 25, 2019
* 8.0 Release: running changelog

* Changelog: add #13921

* Changelog: add #13980

* Changelog: add #13905

* Changelog: add #13971

* Changelog: add #13984

* Changelog: add #14009

* Changelog: add #13620

* Remove things that will ship in 7.9.1

* Changelog: add 7.9.1 release (#14044)

* Changelog: add base for 7.9.1 release

* Update release date and post link

* Changelog: add #14066

* Update changelog for 7.9.1

* Changelog: add #13405

* Changelog: add #13841

* Changelog: add #13924

* Changelog: add #13986

* Changelog: add #14010, #14028, #14053, #14055.

* Changelog: add #14054

* Changelog: add #14031

* Changelog: add #14039

* Changelog: add #14050

* Changelog: add #14070

* Changelog: add #14082

* Changelog: add #14084

* Changelog: add #14111

* Changelog: add #13961

* Changelog: add #14047

* Changelog: add #14091

* Changelog: add #14108

* Changelog: add #14121
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Photon aka "Image CDN". Feature developed in the Image CDN package and shipped in multiple plugins [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants