Skip to content

Responsive Videos: Don't add Jetpack video wrapper for video embeds if WordPress core supports responsiveness for them already#8225

Merged
zinigor merged 1 commit intomasterfrom
fix/video-widgets-responsiveness
Nov 22, 2017
Merged

Responsive Videos: Don't add Jetpack video wrapper for video embeds if WordPress core supports responsiveness for them already#8225
zinigor merged 1 commit intomasterfrom
fix/video-widgets-responsiveness

Conversation

@oskosk
Copy link
Copy Markdown
Contributor

@oskosk oskosk commented Nov 21, 2017 •
edited
Loading

Fixes #8215 for Jetpack sites running WordPress 4.9 and with a theme that supports the jetpack-responsive-video features.

Sequel to r165801-wpcom.

WordPress 4.9 already provides responsiveness for video embeds and widgets, so this PR makes Jetpack not attempt to achieve responsiveness for them too.

Testing instructions:

  • Start with a connected Jetpack site running on WordPress 4.9.
    • Checkout this branch.
    • Activate theme Twenty Sixteen or any other that support jetpack-responsive-videos on the site.
    • Add a Video Widget with a Vimeo video.
    • Add a Video Widget with a Youtube video.
    • On the customizer add this CSS:
    .site-inner {
  max-width: 5000px ;
}
  • With Chrome Dev Tools, try to emulate a device with resolution like 4500x1800.
  • Confirm that both videos play well without going into fullscreen. Audio and video should be perceived.
  • Confirm that going fullscreen in videos fills the whole screen and play well.

@oskosk oskosk added [Feature] Theme Tools [Status] Needs Review This PR is ready for review. Bug When a feature is broken and / or not performing as intended labels Nov 21, 2017
@oskosk oskosk requested a review from a team as a code owner November 21, 2017 22:51
@oskosk oskosk changed the title Responsive Videos: Don't add jetpack wrapper for Video embeds if WordPress core supports responsiveness for them already Responsive Videos: Don't add Jetpack Video wrapper for Video embeds if WordPress core supports responsiveness for them already Nov 21, 2017
@oskosk oskosk mentioned this pull request Nov 21, 2017
Closed
@oskosk oskosk changed the title Responsive Videos: Don't add Jetpack Video wrapper for Video embeds if WordPress core supports responsiveness for them already Responsive Videos: Don't add Jetpack video wrapper for video embeds if WordPress core supports responsiveness for them already Nov 21, 2017
@oskosk oskosk mentioned this pull request Nov 22, 2017
Closed
@oskosk oskosk requested review from jeherve and zinigor November 22, 2017 01:21
zinigor
zinigor approved these changes Nov 22, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@zinigor zinigor left a comment

Choose a reason for hiding this comment

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

Tested this thing all day yesterday. LGTM :)

@zinigor zinigor added this to the 5.5.1 milestone Nov 22, 2017
@zinigor zinigor 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 Nov 22, 2017
@zinigor zinigor merged commit f4d266d into master Nov 22, 2017
@zinigor zinigor deleted the fix/video-widgets-responsiveness branch November 22, 2017 11:36
@zinigor zinigor removed the [Status] Ready to Merge Go ahead, you can push that green button! label Nov 22, 2017
jeherve added a commit that referenced this pull request Nov 22, 2017
@jeherve
Changelog: add #8225
3255545
jeherve pushed a commit that referenced this pull request Nov 22, 2017
@oskosk @jeherve
Don't add jetpack wrapper for Video Widgets if WordPress core supports (
6fe076a 
#8225)

responsiveness for them already (>4.9)
@jeherve
Copy link
Copy Markdown
Member

jeherve commented Nov 22, 2017

Cherry picked to branch-5.5 in 6fe076a

jeherve pushed a commit that referenced this pull request Nov 22, 2017
@zinigor @jeherve
Running changelog for 5.5.1 (#8217)
1edd96b 
* Added changelog for #8201 and #8177.

* Changelog 5.5.1: create base for changelog.

* Fix typo and update release post link.

* Changelog: add #8167

* Changelog: add #8204

* Changelog: add #8129

* Changelog: add #8225

* Changelog: add #8219
jeherve pushed a commit that referenced this pull request Nov 22, 2017
@zinigor @jeherve
Running changelog for 5.5.1 (#8217)
cadafb3 
* Added changelog for #8201 and #8177.

* Changelog 5.5.1: create base for changelog.

* Fix typo and update release post link.

* Changelog: add #8167

* Changelog: add #8204

* Changelog: add #8129

* Changelog: add #8225

* Changelog: add #8219
@pbuzdin
Copy link
Copy Markdown

pbuzdin commented Aug 3, 2019

it is still broken

@jeherve
Copy link
Copy Markdown
Member

jeherve commented Aug 5, 2019

@pbuzdin If you still experience issues, could you please create a new issue, tell us more about your setup (version of WP, version of Gutenberg if it is installed on your site, version of Jetpack, theme and theme version), and give some steps so we can reproduce the issue?

Thank you.

@pbuzdin
Copy link
Copy Markdown

pbuzdin commented Aug 6, 2019

@jeherve I created issue #13187

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeherve jeherve Awaiting requested review from jeherve

1 more reviewer

@zinigor zinigor zinigor approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Bug When a feature is broken and / or not performing as intended [Feature] Theme Tools Touches WP.com Files

Projects

None yet

Milestone

5.5.1

Development

Successfully merging this pull request may close these issues.

Video Widget: videos inserted from URL have wonky spacing

5 participants

@oskosk @jeherve @pbuzdin @zinigor @matticbot