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

Prevent AMP validation errors from unnecessary code in responsive-videos module extra #13205

Merged
merged 1 commit into from Aug 21, 2019
Merged

Prevent AMP validation errors from unnecessary code in responsive-videos module extra #13205

merged 1 commit into from Aug 21, 2019

Conversation

westonruter
Copy link
Collaborator

@westonruter westonruter commented Aug 9, 2019 •
edited

The responsive-videos module extra can cause an AMP validation error (courtesy @amedina):

image

Part of #9730.

Changes proposed in this Pull Request:

  • Short-circuit logic in responsive-videos when generating AMP pages since AMP does not allow custom scripts and videos are inherently responsive in AMP.

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

Testing instructions:

  • Install official AMP plugin.
  • Activate Twenty Nineteen (or another theme that has jetpack-responsive-videos theme support)
  • Switch to Standard or Transitional modes.
  • Add a YouTube video embed block to post content.
  • Click Save Draft.
  • No validation error warning notice should appear, and the previous validation errors (above) should be cleared from the Validated URL screen (below).

image

Proposed changelog entry for your changes:

  • Improve AMP compatibility by preventing unnecessary responsive-videos logic from running.

@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 206317f

@westonruter westonruter marked this pull request as ready for review August 9, 2019 22:57
@westonruter westonruter requested a review from a team as a code owner August 9, 2019 22:57
@westonruter westonruter mentioned this pull request Aug 9, 2019
Open
@kbrown9 kbrown9 added [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it AMP labels Aug 15, 2019
kbrown9
kbrown9 approved these changes Aug 15, 2019
View reviewed changes
Copy link
Member

@kbrown9 kbrown9 left a comment

Choose a reason for hiding this comment

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

I tested the changes using the provided testing instructions, and everything looks good! The AMP validation errors were gone, and the video was responsive. (I had to deactivate the Notifications module to completely clear the AMP validation errors.)

@kbrown9 kbrown9 added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Aug 15, 2019
@jeherve jeherve added this to the 7.7 milestone Aug 21, 2019
@matticbot
Copy link
Contributor

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

jeherve
jeherve approved these changes Aug 21, 2019
View reviewed changes
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. 👍

@jeherve jeherve merged commit 785860f into Automattic:master Aug 21, 2019
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Aug 21, 2019
jeherve added a commit that referenced this pull request Aug 23, 2019
@jeherve
changelog: add #13205
4b09370
jeherve added a commit that referenced this pull request Aug 27, 2019
@jeherve
7.7 changelog: initial set of changes (#13190)
190301b 
* 7.7 changelog: initial set of changes

* Changelog: add #13154

* Changelog: add #13134

* Changelog: add #12699 and many others

* Changelog: add #13127

* Changelog: add #13167

* Changelog: add #13225

* Changelog: add #13179

* Changelog: add #13173

* Changelog: add #13232

* Changelog: add #13137

* Changelog: add #13172

* Changelog: add #13182

* Changelog: add #13200

* Changelog: add #13244

* Changelog: add #13267

* Changelog: add #13204

* changelog: add #13205

* Changelog: add #13183

* Changelog: add #13278

* Changelog: add #13162

* Changelog: add #13268

* Changelog: add #13286

* Changelog: add #13273

* Changelog: add #12474

* Changelog: add #13085

* Changelog: add #13266

* Changelog: add #13306

* Changelog: add #13311

* Changelog: add #13302

* Changelog: add #13196

* Changelog: add #12733

* Changelog: add #13261

* Changelog: add #13322

* Changelog: add #13333

* Changelog: add #13335
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeherve jeherve jeherve approved these changes

@kbrown9 kbrown9 kbrown9 approved these changes

Assignees
No one assigned
Labels
AMP Touches WP.com Files [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Milestone
7.7
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@westonruter @jetpackbot @matticbot @jeherve @kbrown9