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 an admin pointer for version 1.0 #1271

Merged
merged 5 commits into from
Jul 26, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions amp.php
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
47 changes: 47 additions & 0 deletions assets/js/amp-admin-pointer.js
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 ) );
113 changes: 113 additions & 0 deletions includes/admin/class-amp-admin-pointer.php
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';
Copy link
Contributor Author

@kienstra kienstra Jul 19, 2018

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.


/**
* 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 &#8220;Paired&#8221; and &#8220;Native&#8221; modes.', 'amp' )
),
'position' => array(
'edge' => 'left',
'align' => 'middle',
),
),
),
);
}
}
10 changes: 10 additions & 0 deletions includes/admin/functions.php
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This bootstrapping is repetitive. It'd be nice if amp_post_meta_box(), amp_editor_core_blocks(), and amp_admin_pointer() could be combined into one function, like amp_admin_bootstrap().

But in the unlikely case that a user unhooked one of these from wp_loaded, this would break that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then the questions are:

  • Do we need to provide the ability to unhook any of those components?
  • Do we need to provide the ability to unhook any of these components?
	add_action( 'wp_loaded', 'amp_editor_core_blocks' );
	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' );

Changing that to add_action( 'wp_loaded', 'amp_admin_bootstrap' ); and then letting amp_admin_bootstrap() manage the order and initializations results in less callbacks having to be processed, i.e. only 1 instead of 5. It'll be slightly faster.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 amp_admin_pointer() to amp_admin_bootstrap(), without making any other change.

Then, any future admin classes could add their bootstrapping there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another option is renaming amp_admin_pointer() to amp_admin_bootstrap(), without making any other change.

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.

Copy link
Contributor Author

@kienstra kienstra Jul 20, 2018

Choose a reason for hiding this comment

The 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 1.0.

amp_post_meta_box() was added in 0.6. I'm not sure why someone would want to hide the meta box section entirely, and remove the ability to change the AMP enabled status:

amp-meta-box-section

Maybe I could only combine amp_editor_core_blocks() and amp_admin_pointer() into amp_admin_bootstrap().

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, Tonya.

Copy link
Member

Choose a reason for hiding this comment

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

1 change: 1 addition & 0 deletions includes/class-amp-autoloader.php
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
114 changes: 114 additions & 0 deletions tests/test-class-amp-admin-pointer.php
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'] );
}
}