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 page support #825

Merged
merged 13 commits into from Dec 8, 2017

Conversation

Projects
None yet
3 participants
@ThierryA
Collaborator

ThierryA commented Dec 7, 2017

This PR adds support for pages, refer to the ACs for more info...

Fixes #176.
Closes #188.
Closes #816.
Closes #619.

Inspired by #188 & #816
Kudos to @technosailor, @mjangda and @mpszone for their contribution.

@ThierryA ThierryA requested a review from westonruter Dec 7, 2017

westonruter added some commits Dec 6, 2017

Fix amp_get_permalink() for permalinks containing query params (e.g. …
…draft and previews)

* Use query var in permalinks containing query vars, instead of endpoint slug.
* Remove needless and unslightly 1 value.
* Add tests.
'name' => $this->get( 'blog_name' ),
),
'headline' => $post_title,
'datePublished' => date( 'c', $post_publish_timestamp ),

This comment has been minimized.

@westonruter

westonruter Dec 8, 2017

Collaborator

These Schema.org properties are also relevant to WebPage entities: http://schema.org/WebPage

So I don't see why we need to limit them just to posts.

This comment has been minimized.

@ThierryA

ThierryA Dec 8, 2017

Collaborator

@westonruter I would argue that. This really depends on the use cases but in most cases pages published/modified date are not relevant, or even inconvenient. It is a very common use case not to include the datePublished and datePublished. If we take Apple for example, we can see that datePublished is used on news article but not on pages.

This comment has been minimized.

@westonruter

westonruter Dec 8, 2017

Collaborator

True, but this is just in regards to the template rendering of the data. We're talking about metadata here. It should be up to the application to decide whether or not to show the datePublished if the type is WebPage. In other words, the WP REST API for pages includes the published date, and that I think is a better parallel to the Schema.org metadata here.

This comment has been minimized.

@ThierryA

ThierryA Dec 8, 2017

Collaborator

Sounds good, I am on board 😄

</p>
<a href="#top" class="back-to-top"><?php esc_html_e( 'Back to top', 'amp' ); ?></a>
</div>
</footer>
<?php do_action( 'amp_post_template_footer', $this ); ?>

This comment has been minimized.

@westonruter

westonruter Dec 8, 2017

Collaborator

Putting this inside of footer will be problematic for any themes that have their own existing footer.php override.

This comment has been minimized.

@ThierryA

ThierryA Dec 8, 2017

Collaborator

Great thoughts!

@westonruter westonruter changed the title from [WIP] #176: add page support to Add page support Dec 8, 2017

@westonruter westonruter referenced this pull request Dec 8, 2017

Closed

Support for Pages #176

@westonruter westonruter requested a review from amedina Dec 8, 2017

@westonruter

This comment has been minimized.

Collaborator

westonruter commented Dec 8, 2017

Merging this is dependent on @amedina confirming with the Google search team that we can reliably use amp query parameters. We may need to add a rel=canonical link.

@ThierryA

Thanks @weston, here is the CR and enhancement.

'name' => $this->get( 'blog_name' ),
),
'headline' => $post_title,
'datePublished' => date( 'c', $post_publish_timestamp ),

This comment has been minimized.

@ThierryA

ThierryA Dec 8, 2017

Collaborator

@westonruter I would argue that. This really depends on the use cases but in most cases pages published/modified date are not relevant, or even inconvenient. It is a very common use case not to include the datePublished and datePublished. If we take Apple for example, we can see that datePublished is used on news article but not on pages.

<div class="amp-status-actions">
<a href="#amp_status" class="save-amp-status hide-if-no-js button"><?php esc_html_e( 'OK', 'amp' ); ?></a>
<a href="#amp_status" class="cancel-amp-status hide-if-no-js button-cancel"><?php esc_html_e( 'Cancel', 'amp' ); ?></a>
<?php if ( $available ) : ?>

This comment has been minimized.

@ThierryA

ThierryA Dec 8, 2017

Collaborator

I really like that approach, in fact I think we could apply the same for CPT with support disabled. That said, I think it isn't really clear for the user as why it is disabled and why the ability to edit the status is removed. I would suggest to keep the edit link and display a warning when toggled (no visual noise on load, informative on toggle)? Here is a commit which implements it, the warning may be more target if need be and if time allows.

</fieldset>
<?php else : ?>
<div class="inline notice notice-warning">
<p><?php esc_html_e( 'AMP cannot be enabled on home page, front page, password protected posts and post types which do not support AMP.', 'amp' ); ?></p>

This comment has been minimized.

@westonruter

westonruter Dec 8, 2017

Collaborator

I think this message should only be shown if the post is the homepage, page for posts, password protected posts, or posts that don't support AMP. In other cases, we should show a different message such as when a plugin blocks AMP for a post via the amp_skip_post filter. For example:

A plugin or theme has disabled AMP support.

This comment has been minimized.

@westonruter

westonruter Dec 8, 2017

Collaborator

Also, the translation string should be changed to replace “front page” with “page for posts”. You can also use “homepage” instead of having a space in there.

Per core:
image

This comment has been minimized.

@westonruter

westonruter Dec 8, 2017

Collaborator

Also probably should say that AMP cannot yet be enabled for those certain pages. Our intention is to eventually allow every page to be AMPed.

This comment has been minimized.

@ThierryA

ThierryA Dec 8, 2017

Collaborator

Thanks for your feedback @westonruter. I improved the error messages to be more accurate in this commit. I had to add amp_post_supports_error() function to avoid redoing all checks.

screen shot 2017-12-08 at 8 16 00 pm

screen shot 2017-12-08 at 8 16 20 pm

screen shot 2017-12-08 at 8 16 34 pm

screen shot 2017-12-08 at 8 16 59 pm

@amedina

Looks pretty good to me. Do tests pass all the AC?

@ThierryA

This comment has been minimized.

Collaborator

ThierryA commented Dec 8, 2017

Thanks @amedina, yes all AC are covered in this PR and we should be pretty close to merging and sending this to QA (once this commit is reviewed).

@westonruter

This comment has been minimized.

Collaborator

westonruter commented Dec 8, 2017

@ThierryA I did a bit more refactoring on the latest changes. Since there can be multiple reasons for why a post does not support AMP:

image

@amedina Ready for your

@ThierryA

This comment has been minimized.

Collaborator

ThierryA commented Dec 8, 2017

Great @weston, all latest changes are good to go codewise from my perspective.

@westonruter westonruter merged commit edd7a40 into develop Dec 8, 2017

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 deleted the feature/176-page-support branch Dec 8, 2017

@westonruter

This comment has been minimized.

Collaborator

westonruter commented Dec 9, 2017

Some screenshots:

screen shot 2017-12-08 at 3 57 28 pm

screen shot 2017-12-08 at 3 55 59 pm

screen shot 2017-12-08 at 3 55 30 pm

screen shot 2017-12-08 at 3 54 12 pm

@westonruter westonruter added this to the v0.6 milestone Dec 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment