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

Add post preview for AMP and allow AMP to be disabled on a per-post basis #813

Merged
merged 16 commits into from Dec 7, 2017

Conversation

Projects
None yet
2 participants
@ThierryA
Copy link
Collaborator

commented Nov 27, 2017

This PR adds the AMP preview button on posts which have AMP support.

amp preview button

Refer to the ACs for more info...

Merges PR #812. This PR is branched off feature/799-post-preview (#812) which is currently pending for review. Once #812 is reviewed and merged, the develop must be merged into this branch and this PR may be reviewed.

Fixes #709.
Fixes #799.

ThierryA added some commits Nov 24, 2017

@ThierryA ThierryA requested review from westonruter and amedina Nov 27, 2017

&&
isset( $post->post_type )
&&
post_type_supports( $post->post_type, AMP_QUERY_VAR )

This comment has been minimized.

Copy link
@ThierryA

ThierryA Nov 27, 2017

Author Collaborator

This differ from the acceptance criteria, I would suggest to add the ability to disable AMP on a per post basis for all post types which have AMP post type support set, not only pages (especially now that WordPress Core has Post Type Templates). Thoughts @amedina @westonruter?

This comment has been minimized.

Copy link
@ThierryA

ThierryA Nov 27, 2017

Author Collaborator

This differ from from the acceptance criteria (point 9) in a sense that the AMP status option would not be displayed at all if the post type does not have AMP support set to avoid visual noise. The thinking behind this is that the AMP status (enabled/disabled) should not be editable for post types (page included) if it doesn't have AMP support set.

If we really want to show the status as disabled, one approach would be to change to edit section with a text indicating that AMP post type support is not enabled for the current post with a link to the AMP settings page where user may enable the post type support.

Thoughts @amedina @westonruter?

This comment has been minimized.

Copy link
@westonruter

westonruter Nov 27, 2017

Member

@ThierryA I think you're right. The AMP status toggle shouldn't be shown at all if the post type doesn't support AMP to begin with.

@westonruter westonruter changed the base branch from develop to feature/799-post-preview Nov 27, 2017

@westonruter

This comment has been minimized.

Copy link
Member

commented Nov 27, 2017

I've temporarily changed the base branch from develop to feature/799-post-preview for the sake of review.

* Render AMP status.
*
* @since 0.6
* @param object $post \WP_POST object.

This comment has been minimized.

Copy link
@westonruter

westonruter Nov 27, 2017

Member

This line should read:

* @param WP_Post $post Current post.
update_post_meta(
$post_id,
self::POST_META_KEY,
sanitize_key( wp_unslash( $_POST[ self::POST_META_KEY ] ) )

This comment has been minimized.

Copy link
@westonruter

westonruter Nov 27, 2017

Member

Note that this line will continue to be valid in WPCS once WordPress-Coding-Standards/WordPress-Coding-Standards#1222 is merged because sanitize_key() is marked in that PR as an autoSlashingFunctions (because it doesn't return any chars that will include slashes).

This comment has been minimized.

Copy link
@ThierryA

ThierryA Dec 1, 2017

Author Collaborator

Good to know 😄 Would you like to have it removed and add a whitelisting comment until it is merged?

* The nonce action.
*
* @since 0.6
* @const string

This comment has been minimized.

Copy link
@westonruter

westonruter Nov 27, 2017

Member

I think @var is actually what should be used here, counter-intuitively. See https://stackoverflow.com/a/18446766/93579

background-size: 17px;
width: 17px;
height: 17px;
margin: 0 8px 0 1px;

This comment has been minimized.

Copy link
@westonruter

westonruter Nov 27, 2017

Member

Looks like some inconsistent indentation here.

}
?>
<div class="misc-pub-section misc-amp-status">
<i></i>

This comment has been minimized.

Copy link
@westonruter

westonruter Nov 27, 2017

Member

Why an i here and not a span?

This comment has been minimized.

Copy link
@ThierryA

ThierryA Dec 3, 2017

Author Collaborator

I guess there have been a debate for a while as "what should be used for icons". Font awesome, Facebook etc. exclusively use the i tag for icons for example. I am personally in favor of using the i tag because it adds a semantic meaning (even though it doesn't mean icon per se) while span doesn't.
That said, the various places in the WP admin which are not using the dashicon font use the span tag so I think it would be better to keep it as consistent as possible. This commit changes the i tag to a span.

@@ -23,6 +23,11 @@ function post_supports_amp( $post ) {
return false;
}
// Listen to post meta.
if ( ! isset( $post->ID ) || 'disabled' === get_post_meta( $post->ID, AMP_Post_Meta_Box::POST_META_KEY, true ) ) {
return false;

This comment has been minimized.

Copy link
@westonruter

westonruter Nov 27, 2017

Member

Question: instead of hard-coding a return false here, what if we instead here did:

$skip = true;

If the default value for $skip were false, then this $skip could then be passed in below as value for the amp_skip_post filter. This would allow plugins to still be able to override the value.

But maybe that would be a premature optimization. It would also be bad if a user indicates that they want to disable AMP for a given post, but then it still ends up getting AMP enabled on the frontend due to a filter that they don't know about.

This comment has been minimized.

Copy link
@ThierryA

ThierryA Dec 3, 2017

Author Collaborator

But maybe that would be a premature optimization. It would also be bad if a user indicates that they want to disable AMP for a given post, but then it still ends up getting AMP enabled on the frontend due to a filter that they don't know about.

That would justified if we want to allow plugins to overwrite that admin value. That said doing that wouldn't make the admin option reflect the correct value so it would be better for plugins to use the get_post_metadata filter which would cover both, admin and frontend.

&&
isset( $post->post_type )
&&
post_type_supports( $post->post_type, AMP_QUERY_VAR )

This comment has been minimized.

Copy link
@westonruter

westonruter Nov 27, 2017

Member

@ThierryA I think you're right. The AMP status toggle shouldn't be shown at all if the post type doesn't support AMP to begin with.

ThierryA added some commits Nov 28, 2017

@ThierryA

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 3, 2017

@westonruter unless we decide to change this, this PR is ready to go.

@ThierryA ThierryA changed the title [WIP] #709 control AMP status on a per page basis #709 control AMP status on a per page basis Dec 3, 2017

@westonruter westonruter changed the base branch from feature/799-post-preview to develop Dec 6, 2017

@westonruter

This comment has been minimized.

Copy link
Member

commented Dec 6, 2017

Merge conflict needs to be resolved.

westonruter added some commits Dec 6, 2017

Load JS for AMP post meta box if post type supports not if post is no…
…t skipped.

* Restore focus on edit link for AMP status edit for accessibility.
* Do not add AMP preview button if AMP has been disabled.
* Change postmeta to be flag indicating whether AMP is disabled.
* Fix spelling and clean up phpdoc.

@westonruter westonruter force-pushed the feature/709-block-amp-per-page branch from 18ab955 to 4f58949 Dec 7, 2017

@westonruter westonruter referenced this pull request Dec 7, 2017

Closed

Add AMP preview button #812

@westonruter westonruter changed the title #709 control AMP status on a per page basis Add post preview for AMP and allow AMP to be disabled on a per-post basis Dec 7, 2017

@westonruter
Copy link
Member

left a comment

@ThierryA ready for your follow-up review

@ThierryA

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 7, 2017

Thanks @westonruter, all your changes are approved. I pushed two other commits:

  • 802bcfb which improves the preview button styling, specifically making sure that the preview has the same background color (previously white) as the preview button and keep the exact same disabled styling (the border was previously lighter as the entire button opacity was reduced rather than just the background image).
  • 1b6cc54 which is a slight code improvement to apply DRY principals and avoid using and extra variable unnecessary.

Ready for review final review and merge.

@westonruter westonruter merged commit eeb502b into develop Dec 7, 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/709-block-amp-per-page branch Dec 7, 2017

@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
You can’t perform that action at this time.