From 3de8520a9c2873dceaf92d6c39914b3e3bae5d72 Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Tue, 17 Jul 2018 23:19:04 -0500 Subject: [PATCH 1/4] Add an admin pointer for version 1.0 Mainly follow Weston's mockup in Issue 1254. Enqueue the script only if this hasn't been dismissed. Based on: https://code.tutsplus.com/articles/integrating-with-wordpress-ui-admin-pointers--wp-26853 Also uses the scaffold of amp-block-validation.js. --- amp.php | 1 + assets/js/amp-admin-pointer.js | 37 ++++++++ includes/admin/class-amp-admin-pointer.php | 100 +++++++++++++++++++++ includes/admin/functions.php | 10 +++ includes/class-amp-autoloader.php | 1 + tests/test-class-amp-admin-pointer.php | 93 +++++++++++++++++++ 6 files changed, 242 insertions(+) create mode 100644 assets/js/amp-admin-pointer.js create mode 100644 includes/admin/class-amp-admin-pointer.php create mode 100644 tests/test-class-amp-admin-pointer.php diff --git a/amp.php b/amp.php index de6f6507131..9521b6e3620 100644 --- a/amp.php +++ b/amp.php @@ -154,6 +154,7 @@ function amp_init() { add_action( 'wp_loaded', 'amp_post_meta_box' ); add_action( 'wp_loaded', 'amp_editor_core_blocks' ); add_action( 'wp_loaded', 'amp_add_options_menu' ); + add_action( 'wp_loaded', 'amp_admin_pointer' ); add_action( 'parse_query', 'amp_correct_query_when_is_front_page' ); // Redirect the old url of amp page to the updated url. diff --git a/assets/js/amp-admin-pointer.js b/assets/js/amp-admin-pointer.js new file mode 100644 index 00000000000..39d3a3d319d --- /dev/null +++ b/assets/js/amp-admin-pointer.js @@ -0,0 +1,37 @@ +/** + * Adds an admin pointer that describes new features in 1.0. + */ + +/* exported ampAdminPointer */ +/* global ajaxurl, jQuery */ +var ampAdminPointer = ( function( $ ) { // eslint-disable-line no-unused-vars + 'use strict'; + + return { + + /** + * Loads the pointer. + * + * @param {Object} data - Module data. + * @return {void} + */ + load: function load( data ) { + var options = $.extend( + data.pointer.options, + { + /** + * Makes a POST request to store the pointer ID as dismissed for this user. + */ + close: function() { + $.post( ajaxurl, { + pointer: data.pointer.pointer_id, + action: 'dismiss-wp-pointer' + } ); + } + } + ); + + $( data.pointer.target ).pointer( options ).pointer( 'open' ); + } + }; +}( jQuery ) ); diff --git a/includes/admin/class-amp-admin-pointer.php b/includes/admin/class-amp-admin-pointer.php new file mode 100644 index 00000000000..7abd3b64e28 --- /dev/null +++ b/includes/admin/class-amp-admin-pointer.php @@ -0,0 +1,100 @@ +get_pointer_data() ) ) + ); + } + + /** + * Gets the pointer data to pass to the script. + * + * @since 1.0 + * @return array + */ + public function get_pointer_data() { + return array( + 'pointer' => array( + 'pointer_id' => self::TEMPLATE_POINTER_ID, + 'target' => '#toplevel_page_amp-options', + 'options' => array( + 'content' => sprintf( + '

%s

%s

%s

', + __( 'AMP', 'amp' ), + __( 'New AMP Template Modes', 'amp' ), + __( 'You can now reuse your theme\'s styles in AMP responses, using "Paired" or "Native" mode.', 'amp' ) + ), + 'position' => array( + 'edge' => 'left', + 'align' => 'middle', + ), + ), + ), + ); + } +} diff --git a/includes/admin/functions.php b/includes/admin/functions.php index c2c26d8893c..decaeb3e72b 100644 --- a/includes/admin/functions.php +++ b/includes/admin/functions.php @@ -166,3 +166,13 @@ function amp_editor_core_blocks() { $editor_blocks = new AMP_Editor_Blocks(); $editor_blocks->init(); } + +/** + * Bootstrap the AMP admin pointer class. + * + * @since 1.0 + */ +function amp_admin_pointer() { + $admin_pointer = new AMP_Admin_Pointer(); + $admin_pointer->init(); +} diff --git a/includes/class-amp-autoloader.php b/includes/class-amp-autoloader.php index 1a83f70f3d7..04499b7e6b5 100644 --- a/includes/class-amp-autoloader.php +++ b/includes/class-amp-autoloader.php @@ -35,6 +35,7 @@ class AMP_Autoloader { 'AMP_Comment_Walker' => 'includes/class-amp-comment-walker', 'AMP_Template_Customizer' => 'includes/admin/class-amp-customizer', 'AMP_Post_Meta_Box' => 'includes/admin/class-amp-post-meta-box', + 'AMP_Admin_Pointer' => 'includes/admin/class-amp-admin-pointer', 'AMP_Post_Type_Support' => 'includes/class-amp-post-type-support', 'AMP_Base_Embed_Handler' => 'includes/embeds/class-amp-base-embed-handler', 'AMP_DailyMotion_Embed_Handler' => 'includes/embeds/class-amp-dailymotion-embed', diff --git a/tests/test-class-amp-admin-pointer.php b/tests/test-class-amp-admin-pointer.php new file mode 100644 index 00000000000..0bfc3dcfbdf --- /dev/null +++ b/tests/test-class-amp-admin-pointer.php @@ -0,0 +1,93 @@ +instance = new AMP_Admin_Pointer(); + } + + /** + * Test init. + * + * @covers AMP_Admin_Pointer::init() + */ + public function test_init() { + $this->instance->init(); + $this->assertEquals( 10, has_action( 'admin_enqueue_scripts', array( $this->instance, 'enqueue_pointer' ) ) ); + } + + /** + * Test enqueue_pointer. + * + * @covers AMP_Admin_Pointer::enqueue_pointer() + */ + public function test_enqueue_pointer() { + $user_id = $this->factory()->user->create(); + $dismissed_key = 'dismissed_wp_pointers'; + $pointer_script_slug = 'wp-pointer'; + wp_set_current_user( $user_id ); + + // When this is in the meta value of dismissed pointers, the tested method should exit without enqueuing the assets. + update_user_meta( $user_id, $dismissed_key, AMP_Admin_Pointer::TEMPLATE_POINTER_ID ); + $this->instance->enqueue_pointer(); + $this->assertFalse( wp_style_is( $pointer_script_slug ) ); + $this->assertFalse( wp_script_is( AMP_Admin_Pointer::SCRIPT_SLUG ) ); + + // When this isn't in the meta value of dismissed pointers, the method should enqueue the assets. + update_user_meta( $user_id, $dismissed_key, 'foo-pointer' ); + $this->instance->enqueue_pointer(); + $script = wp_scripts()->registered[ AMP_Admin_Pointer::SCRIPT_SLUG ]; + + $this->assertTrue( wp_style_is( $pointer_script_slug ) ); + $this->assertTrue( wp_script_is( AMP_Admin_Pointer::SCRIPT_SLUG ) ); + $this->assertEquals( array( 'jquery', 'wp-pointer' ), $script->deps ); + $this->assertEquals( AMP_Admin_Pointer::SCRIPT_SLUG, $script->handle ); + $this->assertEquals( amp_get_asset_url( 'js/amp-admin-pointer.js' ), $script->src ); + $this->assertEquals( AMP__VERSION, $script->ver ); + $this->assertContains( 'ampAdminPointer.load(', $script->extra['after'][1] ); + } + + /** + * Test get_pointer_data. + * + * @covers AMP_Admin_Pointer::get_pointer_data() + */ + public function test_get_pointer_data() { + $pointer_data = $this->instance->get_pointer_data(); + $pointer = $pointer_data['pointer']; + $this->assertContains( '

AMP

New AMP Template Modes

', $pointer['options']['content'] ); + $this->assertEquals( + array( + 'align' => 'middle', + 'edge' => 'left', + ), + $pointer['options']['position'] + ); + $this->assertEquals( AMP_Admin_Pointer::TEMPLATE_POINTER_ID, $pointer['pointer_id'] ); + $this->assertEquals( '#toplevel_page_amp-options', $pointer['target'] ); + } +} From e84e22d3db2c9d43611160aee4174b3e746b8e07 Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Fri, 20 Jul 2018 15:47:13 -0500 Subject: [PATCH 2/4] Apply Tonya's suggestion for pointer styling. Fix issue where pointer didn't appear alongside the AMP menu item. Uses the show function, as Tonya suggested. --- assets/js/amp-admin-pointer.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/assets/js/amp-admin-pointer.js b/assets/js/amp-admin-pointer.js index 39d3a3d319d..ed47ff9f5bd 100644 --- a/assets/js/amp-admin-pointer.js +++ b/assets/js/amp-admin-pointer.js @@ -27,6 +27,16 @@ var ampAdminPointer = ( function( $ ) { // eslint-disable-line no-unused-vars pointer: data.pointer.pointer_id, action: 'dismiss-wp-pointer' } ); + }, + + /** + * Adds styling to the pointer, to ensure it appears alongside the AMP menu. + * + * @param {Object} event The pointer event. + * @param {Object} t Pointer element and state. + */ + show: function( event, t ) { + t.pointer.css( 'position', 'fixed' ); } } ); From e60f0f67fced870d11e9e87690198cc32ad39db8 Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Fri, 20 Jul 2018 18:14:56 -0500 Subject: [PATCH 3/4] Add method is_pointer_dismissed(). Abstract logic from enqueue_pointer() into this. First check that the user meta is not empty. Then, return whether it is in the array. Props @hellofromtonya. --- includes/admin/class-amp-admin-pointer.php | 21 +++++++++--- tests/test-class-amp-admin-pointer.php | 39 +++++++++++++++++----- 2 files changed, 47 insertions(+), 13 deletions(-) diff --git a/includes/admin/class-amp-admin-pointer.php b/includes/admin/class-amp-admin-pointer.php index 7abd3b64e28..f57101370b0 100644 --- a/includes/admin/class-amp-admin-pointer.php +++ b/includes/admin/class-amp-admin-pointer.php @@ -48,10 +48,7 @@ public function init() { * @since 1.0 */ public function enqueue_pointer() { - $dismissed = explode( ',', strval( get_user_meta( get_current_user_id(), 'dismissed_wp_pointers', true ) ) ); - - // Exit if the pointer has been dismissed. - if ( in_array( self::TEMPLATE_POINTER_ID, $dismissed, true ) ) { + if ( $this->is_pointer_dismissed() ) { return; } @@ -71,6 +68,22 @@ public function enqueue_pointer() { ); } + /** + * Whether the AMP admin pointer has been dismissed. + * + * @since 1.0 + * @return boolean + */ + protected function is_pointer_dismissed() { + $dismissed = get_user_meta( get_current_user_id(), 'dismissed_wp_pointers', true ); + if ( empty( $dismissed ) ) { + return false; + } + $dismissed = explode( ',', strval( $dismissed ) ); + + return in_array( self::TEMPLATE_POINTER_ID, $dismissed, true ); + } + /** * Gets the pointer data to pass to the script. * diff --git a/tests/test-class-amp-admin-pointer.php b/tests/test-class-amp-admin-pointer.php index 0bfc3dcfbdf..8d2b90e8dd7 100644 --- a/tests/test-class-amp-admin-pointer.php +++ b/tests/test-class-amp-admin-pointer.php @@ -13,6 +13,13 @@ */ class Test_AMP_Admin_Pointer extends \WP_UnitTestCase { + /** + * The meta key of the dismissed pointers. + * + * @var string + */ + const DISMISSED_KEY = 'dismissed_wp_pointers'; + /** * An instance of the class to test. * @@ -47,18 +54,11 @@ public function test_init() { */ public function test_enqueue_pointer() { $user_id = $this->factory()->user->create(); - $dismissed_key = 'dismissed_wp_pointers'; $pointer_script_slug = 'wp-pointer'; wp_set_current_user( $user_id ); - // When this is in the meta value of dismissed pointers, the tested method should exit without enqueuing the assets. - update_user_meta( $user_id, $dismissed_key, AMP_Admin_Pointer::TEMPLATE_POINTER_ID ); - $this->instance->enqueue_pointer(); - $this->assertFalse( wp_style_is( $pointer_script_slug ) ); - $this->assertFalse( wp_script_is( AMP_Admin_Pointer::SCRIPT_SLUG ) ); - - // When this isn't in the meta value of dismissed pointers, the method should enqueue the assets. - update_user_meta( $user_id, $dismissed_key, 'foo-pointer' ); + // This pointer isn't in the meta value of dismissed pointers, so the method should enqueue the assets. + update_user_meta( $user_id, self::DISMISSED_KEY, 'foo-pointer' ); $this->instance->enqueue_pointer(); $script = wp_scripts()->registered[ AMP_Admin_Pointer::SCRIPT_SLUG ]; @@ -71,6 +71,27 @@ public function test_enqueue_pointer() { $this->assertContains( 'ampAdminPointer.load(', $script->extra['after'][1] ); } + /** + * Test is_pointer_dismissed. + * + * @covers AMP_Admin_Pointer::is_pointer_dismissed() + */ + public function test_is_pointer_dismissed() { + $user_id = $this->factory()->user->create(); + wp_set_current_user( $user_id ); + $method = new ReflectionMethod( 'AMP_Admin_Pointer', 'is_pointer_dismissed' ); + $method->setAccessible( true ); + + // When this pointer is in the meta value of dismissed pointers, this should be true. + update_user_meta( $user_id, self::DISMISSED_KEY, AMP_Admin_Pointer::TEMPLATE_POINTER_ID ); + $this->instance->enqueue_pointer(); + $this->assertTrue( $method->invoke( $this->instance ) ); + + // When this pointer isn't in the meta value of dismissed pointers, this should be false. + update_user_meta( $user_id, self::DISMISSED_KEY, 'foo-pointer' ); + $this->assertFalse( $method->invoke( $this->instance ) ); + } + /** * Test get_pointer_data. * From 7ae4d6e0b078b8e73c945aa7d2a9e6a3e4436fd8 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 25 Jul 2018 18:43:56 -0700 Subject: [PATCH 4/4] Use curly quotes for pointer message --- includes/admin/class-amp-admin-pointer.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/includes/admin/class-amp-admin-pointer.php b/includes/admin/class-amp-admin-pointer.php index f57101370b0..6bceb6b2a92 100644 --- a/includes/admin/class-amp-admin-pointer.php +++ b/includes/admin/class-amp-admin-pointer.php @@ -72,7 +72,7 @@ public function enqueue_pointer() { * Whether the AMP admin pointer has been dismissed. * * @since 1.0 - * @return boolean + * @return boolean Is dismissed. */ protected function is_pointer_dismissed() { $dismissed = get_user_meta( get_current_user_id(), 'dismissed_wp_pointers', true ); @@ -88,7 +88,7 @@ protected function is_pointer_dismissed() { * Gets the pointer data to pass to the script. * * @since 1.0 - * @return array + * @return array Pointer data. */ public function get_pointer_data() { return array( @@ -100,7 +100,7 @@ public function get_pointer_data() { '

%s

%s

%s

', __( 'AMP', 'amp' ), __( 'New AMP Template Modes', 'amp' ), - __( 'You can now reuse your theme\'s styles in AMP responses, using "Paired" or "Native" mode.', 'amp' ) + __( 'You can now reuse your theme\'s templates and styles in AMP responses, in both “Paired” and “Native” modes.', 'amp' ) ), 'position' => array( 'edge' => 'left',