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

Block AMP on specific pages #709

Closed
doodirock opened this issue Jun 8, 2017 · 9 comments

Comments

Projects
5 participants
@doodirock
Copy link

commented Jun 8, 2017

[Edited] Refer to the latest acceptance criteria.


We have a lot of cases where AMP wont work because of some interactive content we have on a particular page. Is there any way to disable amp in the rel link base on a post ID?

@nicopujol

This comment has been minimized.

Copy link

commented Jun 9, 2017

We implemented something similar the other way around: we insert the interactive content via the Ad Inserter plugin on regular pages. The plugin allows you to blacklist (not load) the content for certain pages, in this case we black listed */amp/ for that content not to load on AMP pages.

@doodirock

This comment has been minimized.

Copy link
Author

commented Jun 19, 2017

Great idea, however instead I do not want any AMP page at all to load. Not just the interactive content.

@westonruter

This comment has been minimized.

Copy link
Member

commented Nov 7, 2017

@doodirock There is a filter you can use to disable AMP support for a given post.

Let's say you add a custom field in postmeta called amp_skipped which if it has an non-empty value it should cause the post to get skipped:

image

You can then add support for that custom postmeta by adding the following to your theme or plugin:

add_filter( 'amp_skip_post', function( $skipped, $post_id ) {
	if ( get_post_meta( $post_id, 'amp_skipped', true ) ) {
		$skipped = true;
	}
	return $skipped;
}, 10, 2 );

So then the question remains for this issue as to whether there should be an actual UI in the AMP plugin to offer “Disable AMP paired mode”. There could be a checkbox in the publish metabox, for example, that offered this.

@westonruter

This comment has been minimized.

Copy link
Member

commented Nov 7, 2017

@amedina Having a toggle like this may be critical for adding support for pages (#176), since pages often have ad hoc structure. We could potentially use the opt-out checkbox instead of forcibly skipping AMP for pages that have custom templates selected, as then the user would have control.

@westonruter westonruter added this to the v0.6 milestone Nov 7, 2017

@westonruter westonruter added this to To Do in v0.6 Nov 7, 2017

@westonruter

This comment has been minimized.

Copy link
Member

commented Nov 8, 2017

There could be an interesting crossover with the AMP Validator here and we could essentially auto-populate the checkbox. Consider this:

  1. I add an embed to the content that is not supported by AMP.
  2. I update the post/page.
  3. After updating the source of the AMP page is scraped and sent off to the validator
  4. If there are validation errors, then the AMP checkbox would be unchecked and disabled, and a warning notice could appear notifying them that AMP is not available because of certain embeds.
  5. After removing the offending embeds and saving, then the validation would happen again and if it passes, the warning would be removed and the checkbox would again be enabled, giving the user the option to forcibly override it or not.

We would probably need to extend the sanitizer logic to detect when embeds get stripped out entirely so that we'd be able to tell the user exactly what is not compatible with AMP.

@westonruter

This comment has been minimized.

Copy link
Member

commented Nov 8, 2017

In fact, this auto-detection of validation could potentially be used instead of introducing a checkbox at all. It could be entirely determined by validation. This would allow posts/pages to eventually start automatically becoming AMP-compatible once the plugin adds support for more and more embeds.

@amedina

This comment has been minimized.

Copy link
Member

commented Nov 8, 2017

I like the thinking here. Still, providing the checkbox may be necessary to enable opting out for specific pages even if they are valid.

@westonruter

This comment has been minimized.

Copy link
Member

commented Nov 13, 2017

Yeah, let's postpone any integration with the AMP validator for a future enhancement.

@ThierryA

This comment has been minimized.

Copy link
Collaborator

commented Nov 23, 2017

Acceptance criteria:

  1. A label will be added within the Publish metabox, “AMP: ”
  2. The label will display whether AMP is enabled or disabled on any post type. “” will either display:
    • “Enabled” if AMP is enabled for the post
    • “Disabled” if AMP is not enabled for the post
  3. Next to the “AMP: ” label will be an “edit” link.
  4. Selecting the “edit” link will expose 2 radio buttons: “enable” and “disable”, along with an “ok” button and “cancel” link.
  5. The radio buttons will default to the current post state selected (e.g. if the post is currently AMP enabled, the “enable” radio button will be selected”
  6. Selecting the “Enable” radio button, followed by the “ok” button will enable AMP for the post.
  7. Selecting the “Disable” radio button, followed by the “ok” button will disable AMP for the post.
  8. Selecting the ”cancel” link will close the edit state and no radio button change selections will be saved.
  9. All posts will default to “enabled” if the “pages” checkbox is selected (enabled) in the AMP settings, see #798. If the “pages” checkbox is deselected (disabled) in the AMP settings, all pages will default to disabled.

This was referenced Nov 23, 2017

@ThierryA ThierryA moved this from To Do to In Progress in v0.6 Nov 27, 2017

@ThierryA ThierryA self-assigned this Nov 27, 2017

@ThierryA ThierryA moved this from In Progress to Ready for Review in v0.6 Dec 3, 2017

@westonruter westonruter moved this from Ready for Review to QA in v0.6 Dec 7, 2017

@csossi csossi moved this from QA to Ready for Merging in v0.6 Dec 10, 2017

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.