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

Tiled Gallery: Ensure Photon strips only 'info', not 'all' when inserting SrcSet #13735

Merged
merged 1 commit into from
Oct 15, 2019

Conversation

jordesign
Copy link
Contributor

Changes proposed in this Pull Request:

This ensures that when SrcSets are generated by the Tiled Gallery they only direct Photon to strip 'Info' instead of 'all' (which removes color profile information).

This addresses issue #13561

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

  • This is a bug fix

Testing instructions:

  • Insert a Tiled Gallery block into a post or page.
  • Upload an image which contains a Color Profile saved within the file. You can use this image to test.
  • Publish the post/page
  • The image within the gallery should appear with the same color brightness/values as you see when loading the original image directly in the browser.

Before (right image has color profile):
Screen Shot 2019-10-12 at 8 51 07 pm

After (right image has color profile):
Screen Shot 2019-10-12 at 8 51 21 pm

Proposed changelog entry for your changes:

  • Color Profile information is retained for images in Tiled Galleries

@jordesign jordesign requested a review from a team October 12, 2019 10:12
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello jordesign! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D33961-code before merging this PR. Thank you!

@jordesign jordesign requested a review from sirreal October 12, 2019 10:12
@jordesign
Copy link
Contributor Author

This is my first PR for Jetpack - but figured as I reported the Issue I should see about fixing it. Totally open to all and any feedback 👍

@jetpackbot
Copy link

Warnings
⚠️

The PR is missing at least one [Status] label. Suggestions: [Status] In Progress, [Status] Needs Review

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 27574f8

@jordesign jordesign added the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Oct 12, 2019
@sirreal
Copy link
Member

sirreal commented Oct 12, 2019

Thanks @jordesign! I suspect there will be an issue with block invalidation with this change. Have you tested creating a Tiled Gallery block without this change, then loading the same post in the block editor with this change? The block will likely be invalidated because save produces different output.

Here's some information about how to deal with that issue if it arises: https://developer.wordpress.org/block-editor/developers/block-api/block-deprecation/

@jordesign
Copy link
Contributor Author

Thanks for mentioning that @sirreal - I wouldn't have thought of it at all.

I've tested that with the branch - and it doesn't appear to be an issue. When inserting the gallery with the Maater branch (and strip="all") and switching to the PR branch (strip="info") - the block still loads in both the front end and the editor. No invalid block 👍

@jeherve jeherve added [Block] Tiled Gallery [Feature] Photon aka "Image CDN". Feature developed in the Image CDN package and shipped in multiple plugins labels Oct 14, 2019
@jeherve jeherve added this to the 7.9 milestone Oct 14, 2019
@jeherve jeherve added [Type] Bug When a feature is broken and / or not performing as intended [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. labels Oct 14, 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 seems to work well for me. Leaving it for @sirreal to give a final approval since he worked on the srcset implementation.

I suspect there will be an issue with block invalidation with this change.

That wasn't an issue in my tests. Could it be because the srcset attribute is not saved in the block html?

@jeherve jeherve added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Oct 14, 2019
Copy link
Member

@sirreal sirreal left a comment

Choose a reason for hiding this comment

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

  • I only see the issue on Simple (WordPress.com) sites, not on Jetpack sites. I wonder if we're observing a difference in Photon implementation 😕
  • I have been unable to reproduce the issue with a Jetpack site, so it's tricky to verify whether this is a fix 🙂
  • I can reproduce the issue on Simple sites, but I'm having issues with my sandbox and haven't been able to test the patch.
  • I do see the expected changes without block invalidation (👍) when I apply this patch to my Jetpack site, so that aspect looks good.

@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. labels Oct 15, 2019
@jeherve jeherve merged commit 354af3d into master Oct 15, 2019
@jeherve jeherve deleted the fix/tiled-gallery-photon-strip branch October 15, 2019 12:24
@jeherve
Copy link
Member

jeherve commented Oct 15, 2019

I can reproduce the issue on Simple sites, but I'm having issues with my sandbox and haven't been able to test the patch.

The patch seems to work on my end on Simple sites. I've committed the change there as well.

@matticbot matticbot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Oct 15, 2019
jeherve added a commit that referenced this pull request Oct 23, 2019
jeherve added a commit that referenced this pull request Oct 29, 2019
* 7.9: Changelog

* Update version number

* Update stable tag and tested up to

* Changelog: add #13530

* changelog: add #13578

* Changelog: add #13598

* Changelog: add entry for numerous block preview changes

* Changelog: add #13599

* changelog: add #13541

* Changelog: add #13542

* Changelog: add #13331

* Changelog: add #13558

* Changelog: add #13409

* Changelog: add #13582

* Changelog: add #13600

* Changelog: add #13601

* Changelog: add #13595

* Changelog: add #12695

* Changelog: add #13009

* Changelog: add #13649

* Changelog: add #13450

* Changelog: add #13507

* Changelog: add #13658

* Changelog: add #13687

* changelog: add #13683

* Changelog: add #9323

* Changelog: add #13681

* Fix typos in readme

* Add link to WordPress Beta Tester plugin

* Changelog: add #13630

* Changelog: add #13695

* Changelog: add #13659

* Changelog: add #13716

* Changelog: add #13664

* Changelog: add #13682

* Changelog: add #13362

* Changelog: add #13563

* Add testing list for #13563

* Changelog: add #13735

* Changelog: add #13752

* Changelog: add #13624

* Changelog: add #13756

* Changelog: add #13745

* Changelog: add #13728

* Changelog: add #13779

* Changelog: add #13699

* Changelog: add #13804

* Changelog: add #13761

* Changelog: add #13637

* Changelog: add #13517

* Changelog: add #13521

* Changelog: add #13729

* Testing list: add testing instructions for #13729

* Changelog: add sync changes

* Changelog: add #13807

* Changelog: add #13654

* Changelog: add #13795

* Changelog: add #13801

* Changelog: add #13818

* Changelog: add #13725

* Changelog: add #13831

* Changelog: add #13516

* Testing list: add Twenty Twenty instructions

* Changelog: add #13799

* Changelog: add #13805

* Changelog: add #13688

* Changelog: add #13830
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Tiled Gallery [Feature] Photon aka "Image CDN". Feature developed in the Image CDN package and shipped in multiple plugins Touches WP.com Files [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants