Skip to content

Responsive videos: do not load when embed blocks are in use#11171

Merged
jeherve merged 2 commits intomasterfrom
fix/responsive-videos-old-embeds
Jan 29, 2019
Merged

Responsive videos: do not load when embed blocks are in use#11171
jeherve merged 2 commits intomasterfrom
fix/responsive-videos-old-embeds

Conversation

@jeherve
Copy link
Copy Markdown
Member

@jeherve jeherve commented Jan 21, 2019 •
edited
Loading

Fixes #11097

Changes proposed in this Pull Request:

This is a different approach than #10880. Instead of shortcircuiting Jetpack responsive videos as soon as we detect that the theme supports core responsive embeds, we hook into each block as it is being rendered. We check if the block is an embed block, and if so we remove the wrapper div that had been added by Jetpack Responsive videos, thus stopping it from affecting the display of videos.
If however all we find are classic blocks (even with videos inside) or shortcodes, we carry on and use Jetpack responsive videos.

Testing instructions:

  • Start by enabling a theme that supports core Responsive embeds, like Twenty Nineteen.
  • In a new post, paste a YouTube video; it will be automatically converted into an embed block with the video. Set that embed to a full width one.
  • Publish your post.
  • On the frontend, you should not see any jetpack-video-wrapper div around the embed. The video should take the full width of your browser.
  • Edit that post. Delete that embed block, and instead insert a classic block. Inside that classic block, paste the same video URL.
  • Save your changes.
  • On the frontend, you should now see the jetpack-video-wrapper div around the embed.

Proposed changelog entry for your changes:

  • Responsive videos: do not apply for videos that benefit from WordPress' own Responsive Embeds solution.

@jeherve
Responsive videos: do not load when embed blocks are in use
045fc8a 
Fixes #11097

This is a different approach than #10880. Instead of shortcircuiting
Jetpack responsive videos as soon as we detect that the theme supports
core responsive embeds, we check the HTML content of each post for blocks.
If we find blocks, and if we find embed blocks among those, we do not use Jetpack responsive videos.
If however all we find are classic blocks (even with videos inside) or shortcodes, we carry on and use
Jetpack responsive videos.
@jeherve jeherve added Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Theme Tools [Status] Needs Review This PR is ready for review. labels Jan 21, 2019
@jeherve jeherve added this to the 7.0 milestone Jan 21, 2019
@jeherve jeherve self-assigned this Jan 21, 2019
@jeherve jeherve requested a review from a team January 21, 2019 15:55
@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Jan 21, 2019

D23354-code. (newly created revision)

@jetpackbot
Copy link
Copy Markdown
Collaborator

jetpackbot commented Jan 21, 2019 •
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: February 5, 2019.
Scheduled code freeze: January 29, 2019

Generated by 🚫 dangerJS against c4bf355

brbrr
brbrr reviewed Jan 22, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@brbrr brbrr left a comment

Choose a reason for hiding this comment

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

This PR fixes the problem, but it seems that responsive-videos.min.js is still enqueued even when it is not needed.

@jeherve
Copy link
Copy Markdown
Member Author

jeherve commented Jan 22, 2019

@brbrr I have now updated this PR to take a different approach. I've updated the testing instructions accordingly.

brbrr
brbrr approved these changes Jan 22, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@brbrr brbrr left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you :shipit:

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Jan 22, 2019
@jeherve
Copy link
Copy Markdown
Member Author

jeherve commented Jan 22, 2019

@laurelfulford Do you think you could give this a try, see if that solution would work for you?

Thank you!

@jeherve jeherve merged commit 03d45f3 into master Jan 29, 2019
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Jan 29, 2019
@jeherve jeherve deleted the fix/responsive-videos-old-embeds branch January 29, 2019 09:25
jeherve added a commit that referenced this pull request Jan 29, 2019
@jeherve
Changelog: add #11171
18b4863
@laurelfulford
Copy link
Copy Markdown
Contributor

laurelfulford commented Jan 29, 2019

@jeherve Gah, sorry I missed the ping from earlier this week to test!

Looks like things are already moving ahead, but I wanted to confirm I did check this out, and the fix looks great! Thank you for diving into this!

@jeherve jeherve mentioned this pull request May 29, 2019
Closed
@jeherve jeherve mentioned this pull request Sep 16, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@brbrr brbrr brbrr approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jeherve jeherve

Labels

Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Theme Tools Touches WP.com Files

Projects

None yet

Milestone

7.0

Development

Successfully merging this pull request may close these issues.

5 participants

@jeherve @matticbot @jetpackbot @laurelfulford @brbrr