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.

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

Add theme support settings to admin screen; prevent serving dirty AMP #1199

Merged
merged 19 commits into from Jun 8, 2018

Conversation

5 participants
@westonruter
Copy link
Member

westonruter commented Jun 6, 2018

  • Add admin settings for picking between disabled, paired, and native theme support. See #1196.
  • Prevent making AMP Gutenberg blocks available when AMP is not native.
  • Prevent serving dirty AMP by forcibly sanitizing all validation errors when amp_is_canonical(). See #1192.
  • Add checkbox for automatically allowing tree shaking. This is not shown when native/canonical because dirty AMP is not allowed.
  • Add checkbox for automatically sanitizing all validation errors (including tree shaking).
  • Make explicitly clear that unaccepted validation errors will block rendering on AMP
  • When validation errors are automatically sanitized, ensure the terms' term_group is updated when re-checking.
  • Update AMP_Options_Manager::get_options() and to return default values.
  • Prevent showing amphtml link in paired mode when it is known that the URL has blocking validation errors.
  • Add link to General settings admin screen in plugin links.
  • Improve UI for distinguishing between validation errors which are rejected but which are also forced to be sanitized (such as in native AMP mode). These are shown as flagged.
  • Fix: Remove menu-toggle in Twenty Fifteen if there is nothing to open.
  • Add an AMP status item to the admin bar to give easy access to the Invalid URL post for any given URL, with an indicator for whether AMP is available, or else to initiate a validation check if no such post exists.

Fixes #1196.
Fixes #1192.

image

image

image

image

westonruter added some commits Jun 5, 2018

Add options for enabling amp theme support and configuring validation…
… handling

* Add admin settings for picking between disabled, paired, and native theme support. See #1196.
* Add checkbox for automatically allowing tree shaking.
* Add checkbox for automatically sanitizing all validation errors (including tree shaking).
* Make explicitly clear that unaccepted validation errors will block rendering on AMP
* Prevent serving dirty AMP by forcibly sanitizing all validation errors when amp_is_canonical(). See #1192.
* When validation errors are automatically sanitized, ensure the terms' term_group is updated when re-checking.
* Update AMP_Options_Manager::get_options() and to return default values.

@westonruter westonruter added this to the v1.0 milestone Jun 6, 2018

@westonruter westonruter requested review from amedina, ThierryA and kienstra Jun 6, 2018

@kienstra kienstra self-assigned this Jun 6, 2018

@kienstra
Copy link
Collaborator

kienstra left a comment

Approved

Hi @westonruter,
This looks really good, especially the new 'AMP Settings' page. There are a few small points, but no blocker.

if ( 'paired' === $theme_support_option ) {
$args['template_dir'] = './';
}
add_theme_support( 'amp', $args );

This comment has been minimized.

Copy link
@kienstra

kienstra Jun 6, 2018

Collaborator

It's nice how this can force theme support.

</label>
</dt>
<dd>
<?php esc_html_e( 'Display AMP responses in classic (legacy) post templates in a basic design that does not match your theme\'s templates.', 'amp' ); ?>

This comment has been minimized.

Copy link
@kienstra

kienstra Jun 6, 2018

Collaborator

Good description of legacy templating.

<p>
<label for="force_sanitization">
<input id="force_sanitization" type="checkbox" name="<?php echo esc_attr( AMP_Options_Manager::OPTION_NAME . '[force_sanitization]' ); ?>" <?php checked( AMP_Options_Manager::get_option( 'force_sanitization' ) ); ?>>
<?php esc_html_e( 'Automatically accept sanitization for any AMP validation error that is encountered.', 'amp' ); ?>

This comment has been minimized.

Copy link
@kienstra

kienstra Jun 6, 2018

Collaborator

It's great how this conditionally displays on selecting "Paired":
conditional-display

<dt>
<input type="radio" id="theme_support_disabled" name="<?php echo esc_attr( AMP_Options_Manager::OPTION_NAME . '[theme_support]' ); ?>" value="disabled" <?php checked( $theme_support, 'disabled' ); ?> <?php disabled( ! $theme_support_mutable ); ?>>
<label for="theme_support_disabled">
<strong><?php esc_html_e( 'Disabled', 'amp' ); ?></strong>

This comment has been minimized.

Copy link
@kienstra

kienstra Jun 6, 2018

Collaborator

If somebody looked at this really quickly, it might look like AMP is disabled, not just theme support:
classic-templates

Maybe "Disabled" could be "Classic Templates (Disabled)."

This comment has been minimized.

Copy link
@westonruter

westonruter Jun 6, 2018

Author Member

That's a great point. Agreed.

And “Paired” doesn't really make sense to users at first look either, nor does “Native”.

Maybe:

  • Classic Templates (Legacy)
  • Active Theme's Templates with Separate URLs (Paired)
  • Active Theme's Templates without Separate URLs (Canonical/Native)

I don't feel great about these these latter two.

This comment has been minimized.

Copy link
@westonruter

westonruter Jun 6, 2018

Author Member

Renamed Disabled to Classic in 5bae33e. Needs more iteration.

This comment has been minimized.

Copy link
@ThierryA

ThierryA Jun 6, 2018

Collaborator

To look at it from a different angle, perhaps we should consider using an other term than "Theme Support". I understand the reasoning behind it as it is technically defined using add_theme_support() but what this feature really does is controlling the AMP Mode. So perhaps the setting itself should be called "AMP Mode" and then have:

  • Legacy
  • Paired
  • Native

This comment has been minimized.

Copy link
@westonruter

westonruter Jun 6, 2018

Author Member

Yes, that makes perfect sense. 👍

Maybe “Template Mode” would be better since it's clearly related to AMP already.

@@ -43,8 +43,16 @@ public function init() {
if ( function_exists( 'gutenberg_init' ) ) {
add_action( 'enqueue_block_editor_assets', array( $this, 'enqueue_block_editor_assets' ) );
add_filter( 'wp_kses_allowed_html', array( $this, 'whitelist_block_atts_in_wp_kses_allowed_html' ), 10, 2 );
add_filter( 'the_content', array( $this, 'tally_content_requiring_amp_scripts' ) );
add_action( 'wp_print_footer_scripts', array( $this, 'print_dirty_amp_scripts' ) );

This comment has been minimized.

Copy link
@kienstra

kienstra Jun 6, 2018

Collaborator

Do you think the methods tally_content_requiring_amp_scripts() and print_dirty_amp_scripts() could be removed, given that they're not used anymore? Maybe the comment below could link to them.

This comment has been minimized.

Copy link
@westonruter

westonruter Jun 6, 2018

Author Member

You're probably right. It's easy enough to look back in history to find the removal of these methods to resurrect them. 👍

Done in 5bae33e.

westonruter added some commits Jun 6, 2018

@westonruter

This comment has been minimized.

Copy link
Member Author

westonruter commented Jun 6, 2018

In a5756d9 added Settings link to plugins list table:

image

@amedina

amedina approved these changes Jun 6, 2018

Copy link
Member

amedina left a comment

These changes are of paramount importance for success on the adoption of the plugin. Great work.

@westonruter

This comment has been minimized.

Copy link
Member Author

westonruter commented Jun 7, 2018

BTW, the PR regarding URL handling (#1203) is a dependency for the PR regarding the new validation error screen because it ensures that the amp_invalid_url post for a given URL will always be stored with the canonical URL which will then allow for us to reliably add an “AMP Status” item to the admin bar. This is important because if there are unaccepted validation errors on a paired URL then they will get redirected to the non-AMP version and this admin bar item can indicate why. Likewise, we need to omit the amphtml link from the canonical page if there are unaccepted validation errors on the page.

westonruter added some commits Jun 7, 2018

Include HTML comment explaining that amphtml version is not available…
… (and why)

* Indicate the count of validation errors that are blocking AMP from being available.
* Allow passing URL string in addition to amp_invalid_url post to AMP_Invalid_URL_Post_Type::get_invalid_url_validation_errors().

@kienstra kienstra removed their assignment Jun 8, 2018

Introduce "Flagged" status for validation errors which are Rejected b…
…ut which are still forcibly sanitized

* Fix PHP 5.3 error
* Remove automatically setting amp_validation_error term_group based on forced status.
* Let preview URL include development=1 flag.

@westonruter westonruter force-pushed the add/admin-theme-support-options branch from 4f44e7c to d890b6c Jun 8, 2018

@postphotos

This comment has been minimized.

Copy link
Contributor

postphotos commented Jun 8, 2018

This is great stuff, @westonruter! 🎂 🎉 I really like the thought you've put into this.

westonruter added some commits Jun 8, 2018

Improve UI to reflect sanitization status when required
* Update post editor warnings to reflect when validation errors are forcibly sanitized but
not explicitly accepted.
* Allow tree shaking setting to be shown when in native mode.
* Show notice when selecting native mode that sanitization is required.
* Hide the preview button from the amp_invalid_url post if sanitization is required.
Add AMP link to admin bar with indication of AMP status and re-valida…
…te link

Discontinue automatically-deleting an amp_invalid_url post when all errors are resolved.

@westonruter westonruter changed the title [WIP] Add theme support settings to admin screen; prevent serving dirty AMP Add theme support settings to admin screen; prevent serving dirty AMP Jun 8, 2018

@westonruter

This comment has been minimized.

Copy link
Member Author

westonruter commented Jun 8, 2018

Now there is an admin bar menu item to link to the AMP version of the page (when available):

image

As well as the AMP validation status (when it is known):

image

If you click on the AMP link and there are validation errors, you'll get redirected back to the canonical URL with the admin bar updated to indicate this:

image

Clicking on the validate link takes you to the Invalid AMP URL edit post screen which lists out all of the errors so you can take action. This validate admin menu item is present even if there are no known validation errors:

image

@westonruter westonruter merged commit 634fbb8 into develop Jun 8, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter added this to Ready for review in v1.0 Jun 25, 2018

@westonruter westonruter moved this from Ready for review to Ready for QA in v1.0 Jun 25, 2018

@westonruter westonruter deleted the add/admin-theme-support-options branch Jul 5, 2018

@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented Aug 3, 2018

Moving To "Ready For Merging"

At some point, it'd be appropriate to test this. But as work on the Template Modes is ongoing, I'd propose that we don't need to test this now.

But if you have other ideas, feel free to move this back to "Ready For QA."

@kienstra kienstra moved this from Ready for QA to Ready for Merging in v1.0 Aug 3, 2018

@kienstra kienstra moved this from Ready for Merging to In Production in v1.0 Dec 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.