Add capability to generate AMP-only content (AMP-as-Canonical) #668
Conversation
… the html tag, and eliminate 3rd-party JS scripts (Experimental code-final version will be refactored)
Add additional postprocessing actions to remove validation errors.
|
Currently the plugin generates an AMP canonical page with the 2017 Theme; there is one validation error regarding the size of the amp-custom styles; the strategy followed right now is to take the theme styles and inline them as an amp-custom style element but the size of the styles exceed the 50KB limit. |
amp.php
Outdated
| @@ -20,6 +20,9 @@ | |||
| require_once( AMP__DIR__ . '/includes/admin/functions.php' ); | |||
| require_once( AMP__DIR__ . '/includes/settings/class-amp-customizer-settings.php' ); | |||
| require_once( AMP__DIR__ . '/includes/settings/class-amp-customizer-design-settings.php' ); | |||
| require_once( AMP__DIR__ . '/includes/utils/class-amp-dom-utils.php'); | |||
| require_once( AMP__DIR__ . '/option.php' ); | |||
| require_once(AMP__DIR__ . '/post-processing/class-amp-sanitize-tweentyseventeen-theme-plain.php'); | |||
There was a problem hiding this comment.
Add spaces inside parentheses.
…into amedina/amp-canonical
|
I'm not sure but with a first look at the PR code it seems this implementation wouldn't allow to create a Progressive Web AMP website (see #778) which I think is the most important real world use case. The next week I'll have some time to experiment with it and will report back, sorry if I'm missing something at the moment! ;) I'll try to explain the use case concept first: On the "canonical" AMP page, the thunder symbol must not be declared inside the The valid "amphtml" version of page may be served from an AMP cache and it'll miss all the progressive enhancements, but it could use amp-install-serviceworker to preload all the additional JS, CSS and other assets needed from the PWAMP. As the AMP Cache doesn't modify the page link urls, they can link to canonical versions of the page so that the PWAMP will be shown about instantly at the second page view from the same site. Hope I've been clear enough... ;) |
|
@amedina woah, I've tried to merge your branch with develop and couldn't solve the conflicts, could you please update the patches? |
|
We're going to be creating a new PR for canonical support. See discussion after #827 (comment) |
|
(We'll be gleaning the relevant parts from this PR.) |
| // Remove comments | ||
| $minified_css = preg_replace('!/\*[^*]*\*+([^/][^*]*\*+)*/!', '', $minified_css); | ||
| // Remove space after colons | ||
| $minified_css = str_replace(': ', ':', $minified_css); |
There was a problem hiding this comment.
Instead of lines 12 and 14 why not just this?
$minified_css = preg_replace( '#\s+#ms', '', $minified_css);`
| return; | ||
| } | ||
| $index = 0; | ||
| error_log("Recording styles..."); |
There was a problem hiding this comment.
Should we be filling up error_log() with informational messages?
|
|
||
| class AMP_Content { | ||
| private $content; | ||
| private $amp_content = ''; | ||
| private $amp_scripts = array(); | ||
| public $amp_scripts = array(); |
There was a problem hiding this comment.
Why is this made public and exposed?
| @@ -0,0 +1,38 @@ | |||
| <?php | |||
|
|
|||
| class PairedModeActions { | |||
There was a problem hiding this comment.
Why is class name not prefixed with AMP_?
New PR with changes in #425.
Incorporated pending review requests
Created a new branch because to avoid conflicts and continue the work on new branch