Closed westonruter closed 5 years ago
In regards to the video header, I tried using an MP4 on the non-AMP version and it seemed broken:
The video file (VID_20190510_140903.mp4.zip) looks like this:
So I was expecting to see the bridge, not just the top-left corner of the sky. I also didn't expect to see a black margin on the left. So in a6104c3 I fixed what appeared to be a bug with the layout a header video
element, and added AMP compat.
Aside: This also unveiled a bug with the AMP plugin's general handling of video headers, in that the loop
attribute was failing to be added:
--- a/includes/class-amp-theme-support.php
+++ b/includes/class-amp-theme-support.php
@@ -2408,7 +2408,8 @@ class AMP_Theme_Support {
array_merge(
$video_attributes,
array(
- 'src' => $video_settings['videoUrl'],
+ 'src' => $video_settings['videoUrl'],
+ 'loop' => '',
)
)
);
I also noticed another issue with the AMP plugin where if you do not supply a header image at all, then a selected video does not render. For AMP fixes to these issues, see https://github.com/ampproject/amp-wp/pull/2642.
Great. When should this be released on WordPress.org?
@westonruter
This was rolled into the 1.8.7 release which has been available on WordPress.org for the last two hours.
Fixes #77.
This adds
amp
theme support for the official AMP plugin. The existing theme templates and styles are used, but places where JS was used are replaced with the AMP equivalents.amp
theme support withpaired
flag to indicate theme works in both Standard (AMP-first) and Transitional modes (paired AMP).primer_is_amp()
helper function.Primer_Walker_Nav_Menu
with a function that filterswalker_nav_menu_start_el
to inject the submenu toggles. I opted to not use thenav_menu_dropdown
andnav_menu_toggle
theme support flags in favor of just making the changes directly in the theme, as it was actually easier to do so.skip-link-focus-fix
script in favor of inlining the logic, following what was done in Twenty Nineteen.The remaining item to do is to fix the header video when YouTube is used, but this depends on an upstream change to AMP itself: https://github.com/ampproject/amp-wp/pull/2642.