-
Notifications
You must be signed in to change notification settings - Fork 384
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 an admin pointer for version 1.0 #1271
Changes from all commits
3de8520
e84e22d
e60f0f6
7ae4d6e
f8e915b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
/** | ||
* 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' | ||
} ); | ||
}, | ||
|
||
/** | ||
* 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' ); | ||
} | ||
} | ||
); | ||
|
||
$( data.pointer.target ).pointer( options ).pointer( 'open' ); | ||
} | ||
}; | ||
}( jQuery ) ); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,113 @@ | ||
<?php | ||
/** | ||
* Admin pointer class. | ||
* | ||
* @package AMP | ||
* @since 1.0 | ||
*/ | ||
|
||
/** | ||
* AMP_Admin_Pointer class. | ||
* | ||
* Outputs an admin pointer to show the new features of v1.0. | ||
* Based on https://code.tutsplus.com/articles/integrating-with-wordpress-ui-admin-pointers--wp-26853 | ||
* | ||
* @since 1.0 | ||
*/ | ||
class AMP_Admin_Pointer { | ||
|
||
/** | ||
* The ID of the template mode admin pointer. | ||
* | ||
* @var string | ||
*/ | ||
const TEMPLATE_POINTER_ID = 'amp_template_mode_pointer_10'; | ||
|
||
/** | ||
* The slug of the script. | ||
* | ||
* @var string | ||
*/ | ||
const SCRIPT_SLUG = 'amp-admin-pointer'; | ||
|
||
/** | ||
* Initializes the class. | ||
* | ||
* @since 1.0 | ||
*/ | ||
public function init() { | ||
add_action( 'admin_enqueue_scripts', array( $this, 'enqueue_pointer' ) ); | ||
} | ||
|
||
/** | ||
* Enqueues the pointer assets. | ||
* | ||
* If the pointer has not been dismissed, enqueues the style and script. | ||
* And outputs the pointer data for the script. | ||
* | ||
* @since 1.0 | ||
*/ | ||
public function enqueue_pointer() { | ||
if ( $this->is_pointer_dismissed() ) { | ||
return; | ||
} | ||
|
||
wp_enqueue_style( 'wp-pointer' ); | ||
|
||
wp_enqueue_script( | ||
self::SCRIPT_SLUG, | ||
amp_get_asset_url( 'js/' . self::SCRIPT_SLUG . '.js' ), | ||
array( 'jquery', 'wp-pointer' ), | ||
AMP__VERSION, | ||
true | ||
); | ||
|
||
wp_add_inline_script( | ||
self::SCRIPT_SLUG, | ||
sprintf( 'ampAdminPointer.load( %s );', wp_json_encode( $this->get_pointer_data() ) ) | ||
); | ||
} | ||
|
||
/** | ||
* Whether the AMP admin pointer has been dismissed. | ||
* | ||
* @since 1.0 | ||
* @return boolean Is dismissed. | ||
*/ | ||
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. | ||
* | ||
* @since 1.0 | ||
* @return array Pointer data. | ||
*/ | ||
public function get_pointer_data() { | ||
return array( | ||
'pointer' => array( | ||
'pointer_id' => self::TEMPLATE_POINTER_ID, | ||
'target' => '#toplevel_page_amp-options', | ||
'options' => array( | ||
'content' => sprintf( | ||
'<h3>%s</h3><p><strong>%s</strong></p><p>%s</p>', | ||
__( 'AMP', 'amp' ), | ||
__( 'New AMP Template Modes', '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', | ||
'align' => 'middle', | ||
), | ||
), | ||
), | ||
); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This bootstrapping is repetitive. It'd be nice if But in the unlikely case that a user unhooked one of these from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then the questions are:
Changing that to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good questions. I think it'd be best to avoid renaming the functions above, as a user could have unhooked them. But it's not a strongly-held belief, and there probably aren't many users who would have unhooked them. Another option is renaming Then, any future admin classes could add their bootstrapping there. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This option is future-thinking as you are planning for future bootstrapping functionality. That's good. Now, one could argue that confusion may be introduced when a dev is deciding where to bootstrap. Adding an inline comment above the action hook may elevate that confusion. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Though amp_editor_core_blocks() has never been in a released version. It was added in version amp_post_meta_box() was added in Maybe I could only combine There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd vote to review how we want to bootstrap and consider what callbacks we can consolidate. That said, I think it's outside the scope of this particular PR. We may want to open a ticket for it and begin the discussion process beyond ours here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good, Tonya. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I think we should try to get away from bootstrapping like this because the class instantiation variable that gets created is not accessible anywhere. We should instead of an AMP plugin manager that is responsible for instantiating classes and then keeping around the references to the class instances for the plugin (and others) to access later. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,114 @@ | ||
<?php | ||
/** | ||
* Tests for AMP_Admin_Pointer class. | ||
* | ||
* @package AMP | ||
*/ | ||
|
||
/** | ||
* Tests for AMP_Admin_Pointer class. | ||
* | ||
* @covers AMP_Admin_Pointer | ||
* @since 1.0 | ||
*/ | ||
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. | ||
* | ||
* @var AMP_Admin_Pointer | ||
*/ | ||
public $instance; | ||
|
||
/** | ||
* Setup. | ||
* | ||
* @inheritdoc | ||
*/ | ||
public function setUp() { | ||
parent::setUp(); | ||
$this->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(); | ||
$pointer_script_slug = 'wp-pointer'; | ||
wp_set_current_user( $user_id ); | ||
|
||
// 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 ]; | ||
|
||
$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 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. | ||
* | ||
* @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( '<h3>AMP</h3><p><strong>New AMP Template Modes</strong></p>', $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'] ); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you're testing this, you might have to change this value locally. Once you dismiss the pointer, it won't display again. Unless you manually update the user meta value.