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

Remove "Your site does not allow AMP to be disabled" warning #1864

Closed
swissspidy opened this issue Feb 14, 2019 · 6 comments · Fixed by #5010
Closed

Remove "Your site does not allow AMP to be disabled" warning #1864

swissspidy opened this issue Feb 14, 2019 · 6 comments · Fixed by #5010
Assignees
Labels
Changelogged Whether the issue/PR has been added to release notes. Groomed UX WS:Core Work stream for Plugin core
Milestone

Comments

@swissspidy
Copy link
Collaborator

As a user who wants to write a blog post, it's very unexpected to see the "Your site does not allow AMP to be disabled" warning when writing a blog post.

The warning is just shown without any context.

What if you don't even know that the site uses AMP under the hood?

Screenshot:

screenshot 2019-02-14 at 19 21 06

Suggestions/Options:

  • Remove warning completely
  • Show disabled AMP toggle with warning below
  • Show disabled AMP toggle with warning shown in tooltip
@swissspidy swissspidy added the UX label Feb 14, 2019
@westonruter
Copy link
Member

Agreed. That's a strange warning. Since you said it is a paired site, I don't understand why/how a theme could prevent it from being disabled. And only the amp_skip_post filter should be able to prevent AMP from being used on a post, but it wouldn't force AMP to be available.

@westonruter
Copy link
Member

Yes, we should just remove this warning.

But we should actually remove the ability for a theme to prevent AMP from being disabled. The challenge is if a theme depends on AMP components, then disabling AMP will cause problems. But when Bento AMP is a thing and themes can depend on AMP components specifically, then themes can have AMP turned off and reliably use AMP components.

@kmyram kmyram added the WS:UX Work stream for UX/Front-end label May 15, 2020
@pierlon pierlon added WS:Core Work stream for Plugin core and removed WS:UX Work stream for UX/Front-end labels May 21, 2020
@pierlon
Copy link
Contributor

pierlon commented May 21, 2020

@westonruter I think you're mistaken. If AMP has been disabled for a post with the amp_skip_post filter, the error message would be "A plugin or theme has disabled AMP support.".

In this case, the error message "Your site does not allow AMP to be disabled." is shown when a plugin/theme forcibly set's a template as being supported or not via the amp_supportable_templates filter.

But we should actually remove the ability for a theme to prevent AMP from being disabled.

Would this also be applied for plugins?

@westonruter
Copy link
Member

Ah, ok.

Would this also be applied for plugins?

Yes, as I don't think we'd be able to tell the difference between whether a theme or plugin is trying to prevent disabling.

In short, we'd be eliminating the immutable aspect of the supportable templates.

However, at the same time, the notices and the toggle here should only be exposed to users who have developer tools enabled. In other words, the notice could actually remain in place, to some degree but whether the AMP Enabled toggle (or the notice) are displayed as a whole should be gated behind whether a user has “developer tools” enabled, as noted in #2673 (comment).

So this will mean that themes/plugins will continue to specify the default for whether a given template is enabled, and normal users won't be able to make any changes to that… but then users who have access to developer tools (who also have access to the admin screen, for example) can then make the decision for templates should be enabled for AMP. This may conflict with what the theme desires to be enabled/disabled for AMP, but in the coming world of Bento AMP the previous reason for immutable is going away: AMP components will be usable on non-AMP pages, so themes will be able to freely use them in their templates regardless of whether the page is being post-processed.

So long story short: the notices/warnings can actually remain, but we will suppress them and the AMP enabled toggle, if the user does not have developer tools enabled.

@westonruter
Copy link
Member

westonruter commented Jul 9, 2020

Per #2724 (comment), I'm now thinking that we should eliminate a whole bunch of stuff that has turned out to not be useful over the years:

  • The templates_supported flag.
  • The immutable flag for templates supplied via the amp_supportable_templates filter.
  • The paired flag.
  • The available_callback flag (already removed in develop)

@pierlon
Copy link
Contributor

pierlon commented Jul 16, 2020

QA Passed

When a template is disabled, the following (collapsible) notice is now shown:

image

If the user is not an administrator, the notice is not shown.

@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Jul 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelogged Whether the issue/PR has been added to release notes. Groomed UX WS:Core Work stream for Plugin core
Projects
None yet
4 participants