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.
ampproject · Dec 6, 2017 · 4f58949 · 4f58949
1 parent e66e2b2
commit 4f58949
Show file tree

Hide file tree

Showing 6 changed files with 95 additions and 49 deletions.
diff --git a/assets/css/amp-post-meta-box.css b/assets/css/amp-post-meta-box.css
@@ -5,7 +5,7 @@
 */
 
 /* Core preview button */
-#post-preview {
+#post-preview.without-amp {
 	border-top-right-radius: 0;
 	border-bottom-right-radius: 0;
 	float: none;

diff --git a/assets/js/amp-post-meta-box.js b/assets/js/amp-post-meta-box.js
@@ -15,7 +15,11 @@ var ampPostMetaBox = ( function( $ ) {
 		 *
 		 * @since 0.6
 		 */
-		data: {},
+		data: {
+			previewLink: '',
+			disabled: false,
+			statusInputName: ''
+		},
 
 		/**
 		 * Toggle animation speed.
@@ -48,7 +52,9 @@ var ampPostMetaBox = ( function( $ ) {
 		boot: function( data ) {
 			this.data = data;
 			$( document ).ready( function() {
-				this.addPreviewButton();
+				if ( ! this.data.disabled ) {
+					this.addPreviewButton();
+				}
 				this.listen();
 			}.bind( this ) );
 		},
@@ -82,9 +88,11 @@ var ampPostMetaBox = ( function( $ ) {
 		 * @return {void}
 		 */
 		addPreviewButton: function() {
-			$( this.previewBtn )
+			var previewBtn = $( this.previewBtn );
+			previewBtn.addClass( 'without-amp' );
+			previewBtn
 				.clone()
-				.insertAfter( this.previewBtn )
+				.insertAfter( previewBtn )
 				.prop( {
 					'href': this.data.previewLink,
 					'id': this.ampPreviewBtn.replace( '#', '' )
@@ -119,7 +127,7 @@ var ampPostMetaBox = ( function( $ ) {
 		},
 
 		/**
-		 * Add AMP Preview button.
+		 * Add AMP status toggle.
 		 *
 		 * @since 0.6
 		 * @param {Object} $target Event target.
@@ -128,22 +136,27 @@ var ampPostMetaBox = ( function( $ ) {
 		toggleAmpStatus: function( $target ) {
 			var $container = $( '#amp-status-select' ),
 				status = $container.data( 'amp-status' ),
-				$checked;
+				$checked,
+				editAmpStatus = $( '.edit-amp-status' );
 
 			// Don't modify status on cancel button click.
 			if ( ! $target.hasClass( 'button-cancel' ) ) {
-				status = $( '[name="amp_status"]:checked' ).val();
+				status = $( '[name="' + this.data.statusInputName + '"]:checked' ).val();
 			}
 
-			$checked = $( '#amp-satus-' + status );
+			$checked = $( '#amp-status-' + status );
 
 			// Toggle elements.
-			$( '.edit-amp-status' ).fadeToggle( this.toggleSpeed );
+			editAmpStatus.fadeToggle( this.toggleSpeed, function() {
+				if ( editAmpStatus.is( ':visible' ) ) {
+					editAmpStatus.focus();
+				}
+			} );
 			$container.slideToggle( this.toggleSpeed );
 
 			// Update status.
 			$container.data( 'amp-status', status );
-			$checked.prop( 'checked', 'checked' );
+			$checked.prop( 'checked', true );
 			$( '.amp-status-text' ).text( $checked.next().text() );
 		}
 	};

diff --git a/includes/admin/class-amp-post-meta-box.php b/includes/admin/class-amp-post-meta-box.php
@@ -22,20 +22,28 @@ class AMP_Post_Meta_Box {
 	const ASSETS_HANDLE = 'amp-post-meta-box';
 
 	/**
-	 * The post meta key.
+	 * The post meta key for whether the post is skipped.
 	 *
 	 * @since 0.6
 	 * @var string
 	 */
-	const POST_META_KEY = 'amp_status';
+	const DISABLED_POST_META_KEY = 'amp_disabled';
+
+	/**
+	 * The field name for the enabled/disabled radio buttons.
+	 *
+	 * @since 0.6
+	 * @var string
+	 */
+	const STATUS_INPUT_NAME = 'amp_status';
 
 	/**
 	 * The nonce name.
 	 *
 	 * @since 0.6
 	 * @var string
 	 */
-	const NONCE_NAME = 'amp-status';
+	const NONCE_NAME = 'amp-status-nonce';
 
 	/**
 	 * The nonce action.
@@ -61,7 +69,6 @@ public function init() {
 	 * Enqueue admin assets.
 	 *
 	 * @since 0.6
-	 * @return Void Void on failure.
 	 */
 	public function enqueue_admin_assets() {
 		$post     = get_post();
@@ -71,10 +78,9 @@ public function enqueue_admin_assets() {
 			&&
 			'post' === $screen->base
 			&&
-			true === post_supports_amp( $post )
+			post_type_supports( $post->post_type, AMP_QUERY_VAR )
 		);
-
-		if ( true !== $validate ) {
+		if ( ! $validate ) {
 			return;
 		}
 
@@ -95,7 +101,9 @@ public function enqueue_admin_assets() {
 		);
 		wp_add_inline_script( self::ASSETS_HANDLE, sprintf( 'ampPostMetaBox.boot( %s );',
 			wp_json_encode( array(
-				'previewLink' => esc_url_raw( add_query_arg( AMP_QUERY_VAR, true, get_preview_post_link( $post ) ) ),
+				'previewLink'     => esc_url_raw( add_query_arg( AMP_QUERY_VAR, '', get_preview_post_link( $post ) ) ),
+				'disabled'        => (bool) get_post_meta( $post->ID, self::DISABLED_POST_META_KEY, true ),
+				'statusInputName' => self::STATUS_INPUT_NAME,
 			) )
 		) );
 	}
@@ -104,7 +112,7 @@ public function enqueue_admin_assets() {
 	 * Render AMP status.
 	 *
 	 * @since 0.6
-	 * @param object $post WP_POST object.
+	 * @param WP_Post $post Post.
 	 */
 	public function render_status( $post ) {
 		$verify = (
@@ -121,17 +129,13 @@ public function render_status( $post ) {
 			return;
 		}
 
-		$status = get_post_meta( $post->ID, self::POST_META_KEY, true );
-		$labels = array(
+		$disabled = (bool) get_post_meta( $post->ID, self::DISABLED_POST_META_KEY, true );
+		$status   = $disabled ? 'disabled' : 'enabled';
+		$labels   = array(
 			'enabled'  => __( 'Enabled', 'amp' ),
 			'disabled' => __( 'Disabled', 'amp' ),
 		);
 
-		// Set default.
-		if ( empty( $status ) ) {
-			$status = 'enabled';
-		}
-
 		include_once AMP__DIR__ . '/templates/admin/amp-status.php';
 	}
 
@@ -145,7 +149,7 @@ public function save_amp_status( $post_id ) {
 		$verify = (
 			isset( $_POST[ self::NONCE_NAME ] )
 			&&
-			isset( $_POST[ self::POST_META_KEY ] )
+			isset( $_POST[ self::STATUS_INPUT_NAME ] )
 			&&
 			wp_verify_nonce( sanitize_key( wp_unslash( $_POST[ self::NONCE_NAME ] ) ), self::NONCE_ACTION )
 			&&
@@ -157,11 +161,11 @@ public function save_amp_status( $post_id ) {
 		);
 
 		if ( true === $verify ) {
-			update_post_meta(
-				$post_id,
-				self::POST_META_KEY,
-				sanitize_key( wp_unslash( $_POST[ self::POST_META_KEY ] ) )
-			);
+			if ( 'disabled' === $_POST[ self::STATUS_INPUT_NAME ] ) {
+				update_post_meta( $post_id, self::DISABLED_POST_META_KEY, true );
+			} else {
+				delete_post_meta( $post_id, self::DISABLED_POST_META_KEY );
+			}
 		}
 	}
 
@@ -170,8 +174,10 @@ public function save_amp_status( $post_id ) {
 	 *
 	 * Add the AMP query var is the amp-preview flag is set.
 	 *
-	 * @param string $link The post preview link.
 	 * @since 0.6
+	 *
+	 * @param string $link The post preview link.
+	 * @return string Preview URL.
 	 */
 	public function preview_post_link( $link ) {
 		$is_amp = (

diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php
@@ -17,21 +17,39 @@ function amp_get_permalink( $post_id ) {
 	return apply_filters( 'amp_get_permalink', $amp_url, $post_id );
 }
 
+/**
+ * Determine whether a given post supports AMP.
+ *
+ * @since 0.1
+ *
+ * @param WP_Post $post Post.
+ * @return bool Whether the post supports AMP.
+ */
 function post_supports_amp( $post ) {
-	// Because `add_rewrite_endpoint` doesn't let us target specific post_types :(
+
+	// Because `add_rewrite_endpoint` doesn't let us target specific post_types.
 	if ( ! post_type_supports( $post->post_type, AMP_QUERY_VAR ) ) {
 		return false;
 	}
 
-	// Listen to post meta.
-	if ( ! isset( $post->ID ) || 'disabled' === get_post_meta( $post->ID, AMP_Post_Meta_Box::POST_META_KEY, true ) ) {
+	// Skip based on postmeta.
+	if ( ! isset( $post->ID ) || (bool) get_post_meta( $post->ID, AMP_Post_Meta_Box::DISABLED_POST_META_KEY, true ) ) {
 		return false;
 	}
 
 	if ( post_password_required( $post ) ) {
 		return false;
 	}
 
+	/**
+	 * Filters whether to skip the post from AMP.
+	 *
+	 * @since 0.3
+	 *
+	 * @param bool    $skipped Skipped.
+	 * @param int     $post_id Post ID.
+	 * @param WP_Post $post    Post.
+	 */
 	if ( true === apply_filters( 'amp_skip_post', false, $post->ID, $post ) ) {
 		return false;
 	}

diff --git a/templates/admin/amp-status.php b/templates/admin/amp-status.php
@@ -19,11 +19,11 @@
 		<span class="screen-reader-text"><?php esc_html_e( 'Edit Status', 'amp' ); ?></span>
 	</a>
 	<div id="amp-status-select" class="hide-if-js" data-amp-status="<?php echo esc_attr( $status ); ?>">
-		<input id="amp-satus-enabled" type="radio" name="<?php echo esc_attr( self::POST_META_KEY ); ?>" value="enabled"<?php checked( 'enabled', $status ); ?>>
-		<label for="amp-satus-enabled" class="selectit"><?php echo esc_html( $labels['enabled'] ); ?></label>
+		<input id="amp-status-enabled" type="radio" name="<?php echo esc_attr( self::STATUS_INPUT_NAME ); ?>" value="enabled" <?php checked( ! $disabled ); ?>>
+		<label for="amp-status-enabled" class="selectit"><?php echo esc_html( $labels['enabled'] ); ?></label>
 		<br />
-		<input id="amp-satus-disabled" type="radio" name="<?php echo esc_attr( self::POST_META_KEY ); ?>" value="disabled"<?php checked( 'disabled', $status ); ?>>
-		<label for="amp-satus-disabled" class="selectit"><?php echo esc_html( $labels['disabled'] ); ?></label>
+		<input id="amp-status-disabled" type="radio" name="<?php echo esc_attr( self::STATUS_INPUT_NAME ); ?>" value="disabled" <?php checked( $disabled ); ?>>
+		<label for="amp-status-disabled" class="selectit"><?php echo esc_html( $labels['disabled'] ); ?></label>
 		<br />
 		<?php wp_nonce_field( self::NONCE_ACTION, self::NONCE_NAME ); ?>
 		<div class="amp-status-actions">

diff --git a/tests/test-class-amp-meta-box.php b/tests/test-class-amp-meta-box.php
@@ -108,29 +108,38 @@ public function test_render_status() {
 	public function test_save_amp_status() {
 		// Test failure.
 		$post_id = $this->factory->post->create();
-		$this->assertEmpty( get_post_meta( $post_id, AMP_Post_Meta_Box::POST_META_KEY, true ) );
+		$this->assertEmpty( get_post_meta( $post_id, AMP_Post_Meta_Box::DISABLED_POST_META_KEY, true ) );
 
 		// Setup for success.
 		wp_set_current_user( $this->factory->user->create( array(
 			'role' => 'administrator',
 		) ) );
-		$_POST[ AMP_Post_Meta_Box::NONCE_NAME ]    = wp_create_nonce( AMP_Post_Meta_Box::NONCE_ACTION );
-		$_POST[ AMP_Post_Meta_Box::POST_META_KEY ] = 'foo';
+		$_POST[ AMP_Post_Meta_Box::NONCE_NAME ]        = wp_create_nonce( AMP_Post_Meta_Box::NONCE_ACTION );
+		$_POST[ AMP_Post_Meta_Box::STATUS_INPUT_NAME ] = 'disabled';
 
 		// Test revision bail.
 		$post_id = $this->factory->post->create();
-		delete_post_meta( $post_id, AMP_Post_Meta_Box::POST_META_KEY );
+		delete_post_meta( $post_id, AMP_Post_Meta_Box::DISABLED_POST_META_KEY );
 		wp_save_post_revision( $post_id );
-		$this->assertEmpty( get_post_meta( $post_id, AMP_Post_Meta_Box::POST_META_KEY, true ) );
+		$this->assertEmpty( get_post_meta( $post_id, AMP_Post_Meta_Box::DISABLED_POST_META_KEY, true ) );
 
-		// Test post update success.
+		// Test post update success to disable.
 		$post_id = $this->factory->post->create();
-		delete_post_meta( $post_id, AMP_Post_Meta_Box::POST_META_KEY );
+		delete_post_meta( $post_id, AMP_Post_Meta_Box::DISABLED_POST_META_KEY );
 		wp_update_post( array(
 			'ID'         => $post_id,
 			'post_title' => 'updated',
 		) );
-		$this->assertEquals( 'foo', get_post_meta( $post_id, AMP_Post_Meta_Box::POST_META_KEY, true ) );
+		$this->assertTrue( (bool) get_post_meta( $post_id, AMP_Post_Meta_Box::DISABLED_POST_META_KEY, true ) );
+
+		// Test post update success to enable.
+		$_POST[ AMP_Post_Meta_Box::STATUS_INPUT_NAME ] = 'enabled';
+		delete_post_meta( $post_id, AMP_Post_Meta_Box::DISABLED_POST_META_KEY );
+		wp_update_post( array(
+			'ID'         => $post_id,
+			'post_title' => 'updated',
+		) );
+		$this->assertFalse( (bool) get_post_meta( $post_id, AMP_Post_Meta_Box::DISABLED_POST_META_KEY, true ) );
 	}
 
 	/**