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

Support for Pages #176

Closed
mjangda opened this issue Feb 8, 2016 · 10 comments
Closed

Support for Pages #176

mjangda opened this issue Feb 8, 2016 · 10 comments
Assignees
Labels
Enhancement New feature or improvement of an existing one P0 High priority
Milestone

Comments

@mjangda
Copy link
Contributor

mjangda commented Feb 8, 2016

[Edited] Refer to the latest acceptance criteria.


At a minimum, they should have:

  • slightly different template
  • appropriate JSON metadata

We may need to adjust actions/filters to allow targeting posts or pages specifically.

PRs: #188, #619.

@mjangda mjangda added the Enhancement New feature or improvement of an existing one label Feb 8, 2016
@technosailor
Copy link

I'm working on this now. Are there specific areas regarding the JSON schema that you're thinking about?

FWIW, the template renders fine for pages in the work I've done. Not sure much needs to be done to the template outside of removing displayed meta...

@ghost
Copy link

ghost commented Jun 17, 2016

How is support for Pages coming along? I feel like Pages-support is mandatory for this plugin to take off. I'm psyched to give it a shot when support comes, since very few sites that I operate actually use posts.
Also, will Pages-support mean that there will also be support for Custom Post Types?

@ThierryA
Copy link
Collaborator

ThierryA commented Nov 23, 2017

Acceptance criteria:

  1. WordPress core page post-type support will be added.
  2. Similar to how Posts are currently, Pages will be capable of being AMPlified.
  3. A new checkbox will be added to the Settings page (defined in 798), “Pages”.
  4. “Pages” will be checked (enabled) by default, but allow the user to deselect (disable) if they wish to do so.
  5. The settings applied to the “Pages” checkbox on the Settings page will default the AMP status for all pages within the site that are AMPlifiable.
  6. If a specific page has AMP disabled (see rules in Block AMP on specific pages #709), the page will not be AMPlified.
  7. The AMP versions of URLs may include ?amp even with pretty permalinks enabled. This may turn out to be required to play nicely with rewrite rules generated for pages to disambiguate from pages about someone's favorite guitar amp, for example.

@ThierryA ThierryA self-assigned this Nov 27, 2017
@westonruter
Copy link
Member

If a specific page has AMP disabled (see rules in #709), the page will not be AMPlified.

This will be especially important in the case of pages with user-supplied page templates. If a page template has functionality in it that is hard-coded in PHP and showing the page in the AMP template is just not going to be acceptable, then a user would need to disable AMP for the page. We may want to consider defaulting AMP to “disabled” when a custom page template has been selected.

@westonruter
Copy link
Member

One more thing to note on acceptance criteria: the AMP versions of URLs may include ?amp even with pretty permalinks enabled. This may turn out to be required to play nicely with rewrite rules generated for pages to disambiguate from pages about someone's favorite guitar amp, for example.

@ThierryA
Copy link
Collaborator

ThierryA commented Dec 7, 2017

One more thing to note on acceptance criteria: the AMP versions of URLs may include ?amp even with pretty permalinks enabled. This may turn out to be required to play nicely with rewrite rules generated for pages to disambiguate from pages about someone's favorite guitar amp, for example.

@westonruter I added that to the ACs.

@westonruter
Copy link
Member

See PR #825 which is pending merge to resolve this issue.

@csossi
Copy link

csossi commented Dec 10, 2017

Went to Contact Us page at http://amp-xwp-testbed.pantheonsite.io/contact/?amp which has AMP enabled. I ran an AMP Validator extension and it noted the attached error.
image

@westonruter
Copy link
Member

@csossi Thanks for that. This is actually something that I had done as part of #819 but deployed to the testbed prior to merging the PR.

I fixed the issue here: a38d024

Re-deployed and the issue has been resolved: http://amp-xwp-testbed.pantheonsite.io/contact/?amp#development=1

@csossi
Copy link

csossi commented Dec 11, 2017

verified in QA

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 P0 High priority
Projects
None yet
Development

No branches or pull requests

5 participants