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

[SUGGESTION] Regarding 0.7 Progress Feature Enabler #956

Closed
ahmadawais opened this issue Feb 13, 2018 · 6 comments
Closed

[SUGGESTION] Regarding 0.7 Progress Feature Enabler #956

ahmadawais opened this issue Feb 13, 2018 · 6 comments
Labels
Enhancement New feature or improvement of an existing one

Comments

@ahmadawais
Copy link

👍 After trying, beta testing, and watching the AMPConf talk about 0.7 version of AMP-WP I'm super excited about the AMP canonical pages. Love the work you guys have done to improve this plugin. While I understand the need for AMP valid pages — I'd like to suggest that allow theme authors to move towards AMP Canonical in a progressive one feature at a time manner.

✅ E.g. I have a lot of JS in my theme at https://WPCouple.com and I'd love to take advantage of CSS inlining feature and make my theme compatible to go all in on AMP Canonical mode but I'd love to have time to do that, so instead of going all in — I'd want to enable one feature at a time (since I don't want all of my JS stripped out right away, but want to make use of as many AMP components that I can start using now). Let's call it a progressive way of getting towards an all AMP WP theme instead of completely changing the theme to do that.

🤞This way my theme might not be AMP valid but I'd still benefit from the fast page speed. Also, that's what most of the other old themes that are non-amp would want to do.

Would love to hear thoughts of @westonruter @ThierryA and @amedina on this 👍
Peace ✌️

@westonruter
Copy link
Member

I think this resonates with @pbakaus's wishes expressed in #927 (comment):

One additional thought: I'd much rather render invalid AMP pages than broken AMP pages.

On a related note, the plugin currently stops concatenating enqueued CSS if it surpasses 50KB normally. This is what currently causes the site in the Customizer preview to appear unstyled, since the enqueueing of Dashicons surpasses the limit and prevents the theme's style.css from being processed. It would be better to warn the user of the style size overage rather than breaking the theme's design.

Some of the mechanisms to opt-in to dirty styles and scripts can be found in #952.

Some sites will probably want to force the sanitizer to enforce strict AMP validity to catch unintended scripts and styles invalidating their AMP pages. Others, such as yourself, want to explicitly allow for dirty AMP pages. I think the behavior could be managed by a theme support flag, for example:

add_theme_support( 'amp', array(
    'custom_style_limit' => false,
    'remove_dirty_scripts' => false,
) );

That could disable the 50KB limit on styles and prevent the removal of scripts. Some thought would be needed on the best naming for these feature flags.

At the same time, I won't think we want to make it too easy to intentionally create pages that are invalid AMP.

@amedina
Copy link
Member

amedina commented Feb 14, 2018

Agree that we need an approach to enable users to activate 0.7 without breaking their site. Related to this is our discussion in #934. The strategy of opting in into dirty AMP is a possibility. My concern is that such an approach may end up with sites that are always in dirty AMP mode.

@ahmadawais
Copy link
Author

ahmadawais commented Feb 15, 2018

@westonruter I like that approach a lot. I'd 👍 for it to be there in the next version.

@amedina I agree that is possible but not everyone would want to go all in on AMP. E.g. I use this service called Intercom and I'm pretty sure the JS for that service will be stripped out via amp-wp 0.7 sanitizers.

🛠 So, for a site where I can't do without Intercom — I'll be left with the following options

  1. No AMP at all
  2. Separate AMP pages but again no Intercom support so I wonder if that's where I'd wanna go
  3. Dirty AMP pages with just one added-extra JavaScript tag to support intercom and still, benefit from AMP's page speed and canonical features.

✅ I like the third approach specifically because, if we get people onboard with the dirty AMP mode — this is what will/can happen

  • Sites load faster (instead of no AMP at all)
  • Services like Intercom or other 3rd party JS stuff can build integrations for AMP to allow their customers to be able to enjoy 100% AMP valid modes

🤔 IMHO, it's still a better option than no AMP at all — for specific use-cases where a service or two can't co-exist with AMP at the moment.

Thoughts?

@ThierryA
Copy link
Collaborator

ThierryA commented Feb 15, 2018

@ahmadawais I resonate with your concerns and you are most probably not the only only one facing this issue.

I am slightly torn between the two, technically we can definitely add the possibility to do what you are after as @westonruter suggested but I share @amedina concerns that dirty mode will become the easy solution and slow down the efforts to move things forward.

I believe the best would be to put our efforts onto pushing things forward by collaborating with service providers like Intercom to have an AMP version (perhaps an AMP component), that is a real solution for Native AMP from my perspective.

In order to achieve that, support and contributions really help and we are glad that you are helping with that 👍

@westonruter
Copy link
Member

@ahmadawais FYI: I think #1063 is going to end up being the solution to the problem you're facing.

@swissspidy swissspidy added the Enhancement New feature or improvement of an existing one label Jun 14, 2019
@westonruter
Copy link
Member

Closing in favor of #2326 which will default to not excluding excessive CSS on AMP-first sites, and for the custom JS you can just reject the sanitization of those validation errors to prevent them from being removed. The end result is a mostly-AMP page but which lacks the amp attribute on the html element.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or improvement of an existing one
Projects
None yet
Development

No branches or pull requests

5 participants