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

Tiled Gallery: Introduce image srcset #30724

Closed
wants to merge 7 commits into from
Closed

Tiled Gallery: Introduce image srcset #30724

wants to merge 7 commits into from

Conversation

sirreal
Copy link
Member

@sirreal sirreal commented Feb 12, 2019 •
edited
Loading

Add a srcset with a reasonable set of photonized image sizes so that the browser can pick an optimized image.

The image sizes are conservative because we know very little about themes or how the image may be used when we generate the srcset.

This is a fairly naïve implementation, if we push the srcset generation deeper where we know images will be positioned, we could further optimize. That has the potential to add endless complexity 🙂

A deprecated version of the block is added to handle migration to the newer output with srcsets

Resolves #29787
Fixes #30118

Related conversation about allowing some attributes: p3btAN-12W-p2

Known issues

  • WordPress.com sites appear to strip the srcset attribute for some reason. This means that the blocks are always invalidated. D24378-code includes a fix (depends on)

To do

Testing

gutenpack-jn

  • Add a tiled gallery using current master and save.
  • Open the same post with this branch. You should see an "invalid block" warning in the console, but the deprecation handles it.
  • Save the post.
  • Reload ❗️ WordPress.com sites will invalidate the block here ❗️
  • View the gallery on the frontend. Observe the srcset in action

To observe the srcset in action, start with a narrow viewport and observe the image requests (filter img requests /(i[0-2]\.wp\.com)|(files\.wordpress\.com)/ in network).

Increase the browser size and notice that larger assets are requested.

@sirreal sirreal self-assigned this Feb 12, 2019
@matticbot
Copy link
Contributor

Test live: https://calypso.live/?branch=update/tiled-gallery-srcset

@simison simison self-requested a review February 12, 2019 11:11
@sirreal sirreal force-pushed the update/tiled-gallery-srcset branch 4 times, most recently from cf34736 to 9a7cd31 Compare February 14, 2019 12:40
@sirreal sirreal added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Feb 14, 2019
@sirreal sirreal requested a review from a team February 14, 2019 21:49
@simison
Copy link
Member

simison commented Feb 15, 2019

Confirmed that in core srcset (and sizes) attributes get stripped for contributor -level users by wp_kses.

https://github.com/WordPress/WordPress/blob/97cb2375488c717e02aa6872672b97523ffb6d85/wp-includes/kses.php#L227-L238

@sirreal
Copy link
Member Author

sirreal commented Feb 18, 2019 •
edited
Loading

I've put this on hold. There are viable improvements for loading smaller images in the editor that make the block much more responsive to work with. We could optimize further by making assumptions about the editor context within Gutenberg (maximum width, etc).

However, the save/frontend changes are held up by issues with the srcset attribute that is being filtered out.

I'd consider splitting and separating these changes, one set of editor enhancements that improve performance and can be landed in the near future, and another set of changes that improve the frontend. The only concern I'd have there is that the editor would have potentially significant differences from the frontend.

@sirreal sirreal mentioned this pull request Feb 22, 2019
Merged
@sirreal
Copy link
Member Author

sirreal commented Feb 22, 2019

PHP implementation here: Automattic/jetpack#11397

Drop save changes and deprecation from this PR, different view output will be handled server side due to kses issues (srcset attributes stripped for some roles).

@sirreal sirreal removed [Status] Blocked / Hold [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 22, 2019
@sirreal sirreal added this to the Jetpack: 7.1 milestone Feb 22, 2019
@sirreal sirreal mentioned this pull request Feb 25, 2019
Closed
@sirreal
Copy link
Member Author

sirreal commented Feb 25, 2019

Superseded by #30989

@sirreal sirreal closed this Feb 25, 2019
@sirreal sirreal modified the milestones: Jetpack: 7.1, Jetpack: 7.2 Feb 26, 2019
sirreal added a commit that referenced this pull request Feb 28, 2019
@sirreal
Add deprecated
da81c80 
Based on #30724
sirreal added a commit that referenced this pull request Mar 1, 2019
@sirreal
Tiled Gallery: Remove i18n strings in save (#31119)
9803c09 
* Add deprecated declaration based on #30724

* Remove localized aria-label from save
jeherve pushed a commit that referenced this pull request Mar 4, 2019
@sirreal @jeherve
Tiled Gallery: Remove i18n strings in save (#31119)
3c871db 
* Add deprecated declaration based on #30724

* Remove localized aria-label from save
@jeherve jeherve removed this from the Jetpack: 7.2 milestone Mar 5, 2019
jeherve added a commit that referenced this pull request Mar 5, 2019
@jeherve @sirreal
@enejb @jeffersonrabb @roccotripaldi @thomasguillot @simison @mmtr @gititon
Merge pull request #31045 from Automattic/update/jetpack-blocks-71
8c4cb5e 
* Implement image filters for tiled gallery block

* Jetpack Blocks: move a new set of blocks in production

See https://github.com/Automattic/jetpack/milestone/119

* Jetpack Blocks: tag new version of the blocks for the 7.1 release

* Contact Info: Fix/contact info fix sidebar (#31042)

* Contact Info Block: Remove the Sidebar for google maps link

* bug fix: use className instead of class

* Additional keywords for the Slideshow block. (#31038)

* Improvement to Autoplay delay label. (#31039)

* Slideshow: Add New Uploads At End (#31046)

* Add newly uploaded images to the end of the slideshow. Jump to the position of the new upload so user sees upload status.

* Alternate approach using comparison of image array length in componentDidUpdate to determine position.

* Contact Info: Add the ability to add the buisness hours block to the contact info (#31056)

* Contact Info Block: Improve email validation (#31033)

* Contact Info Block: Improve the email validation

By using the emailValidator that is use in more places we can provide better matching and of email addresses.

* bug fix. take into account any number of punctuation.

* Business hours: show 'closed' when data is incomplete for a given interval (#31083)

* Business hours
If data is incomplete for a given interval, display 'Closed' or null;

* adjusting logic - thanks @lezama

* further improved time validation logic

* Contact Info Block: Split phone numbers into prefix and phone number (#31026)

* Split phone numbers into prefix and phone number

This allows us to be not so strict with what a phone number is and still offer a way to prefix. things.

* Fix typos and simplify selection of first character.

* WordAds block: Update icon (#31106)

* Business Hours: Update editor styles for consistency with UI elements

* Business Hours Block: add search keywords (#31090)

* Business Hours Block:

add search keywords

* updating terms after feedback from co-workers

* Related Posts: update the message displayed when no related posts (#31126)

Discussion: p8oabR-kb-p2 #comment-2590

* Tiled Gallery: Remove i18n strings in save (#31119)

* Add deprecated declaration based on #30724

* Remove localized aria-label from save

* Form block: fix typo (#31139)

* Mailchimp: Placeholder Buttons (#31089)

* Rework of button UI hierarchy in unconnected placeholder.

* Change to Connection button styling.

* Add isBorderless to Re-check button.

* Revised button attributes for placeholder.

* Further revisions to button styles.

* Mailchimp: Form Element Spacing (#31142)

* Standardized form element spacing in the editor.

* Use rem for form element spacing.

* Business hours block: use blockIcon in placeholder (#31143)

* Fix: business hours validation (#31122)

* When saving a business hours block, don't save any data that hinges on localization.

* defaultLocalization should not be translated.

* After a conversation with @sirreal - we've decided on this approach:

drop the save component altogether, because we use server-side rendering for the block
have a <DayEdit /> and <DayPreview /> component for client side rendering, which are only rendered once we've fetched localization

* Default days should remain localized.

* Tiled gallery block: camel case product name (#31184)

* VideoPress: Disable re-usable block feature for video blocks (#31186)

* Change contact info icon to account box (#31190)

* Deselect image on filter change

* Update filter icons

via #30853 (comment)

* Slideshow: Replace Buttons With A Tags (#31175)

* Replace Slideshow buttons with A tags. Hide inner text in links. Disable ESLint rule related to anchors without HREFs.

* Remove text from PREV/NEXT/PAUSE buttons, to avoid block invalidation after language change.

* Reposition bullets element below buttons (no visible change).

* Remove localization of text for initial autoplay Pause button state. All a11y label localization will be addressed as a later block of work.

* Removing unneeded CSS now that buttons have no inner content.

* Fix WordAds block alignment

Remove content floating by removing conflicting align{left,right,center}
classes.

Gutenberg styling handles left/right alignment.

Apply center alignment based on data-align attributes.

* Jetpack Blocks: update blocks for the final 7.1 release.


Co-authored-by: Jon Surrell <jon.surrell@automattic.com>
Co-authored-by: Enej Bajgoric <enej.bajgoric@gmail.com>
Co-authored-by: Jefferson Rabb <j.max.rabb@gmail.com>
Co-authored-by: Rocco Tripaldi <tripaldir@gmail.com>
Co-authored-by: Thomas Guillot <thomasguillot@users.noreply.github.com>
Co-authored-by: Mikael Korpela <mikael@ihminen.org>
Co-authored-by: Miguel Torres <miguelmariatorresrojas@gmail.com>
Co-authored-by: Michael Turk <gititon@users.noreply.github.com>
@sirreal sirreal mentioned this pull request Apr 1, 2019
Closed
@lancewillett lancewillett deleted the update/tiled-gallery-srcset branch March 9, 2020 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simison simison Awaiting requested review from simison

Assignees

@sirreal sirreal

Labels
[Block] Tiled Gallery [Goal] Gutenberg Working towards full integration with Gutenberg Jetpack [Type] Enhancement [Type] Performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sirreal @matticbot @simison @jeherve