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

ThierryA
Copy link
Collaborator

@ThierryA ThierryA 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.

&&
isset( $post->post_type )
&&
post_type_supports( $post->post_type, AMP_QUERY_VAR )
Copy link
Collaborator Author

@ThierryA ThierryA Nov 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 November 27, 2017 19:32
@westonruter
Copy link
Member

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ] ) )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this line will continue to be valid in WPCS once WordPress/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).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like some inconsistent indentation here.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why an i here and not a span?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Collaborator Author

ThierryA 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 December 6, 2017 19:11
@westonruter
Copy link
Member

Merge conflict needs to be resolved.

…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 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
Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ThierryA ready for your follow-up review

@ThierryA
Copy link
Collaborator Author

ThierryA 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants