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 1 commit
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
37 changes: 37 additions & 0 deletions assets/js/amp-admin-pointer.js
Original file line number Diff line number Diff line change
@@ -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'
} );
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@kienstra Right now, the pointer scrolls instead of sticking in a fixed position with the AMP menu.

amp-pr1271-pointer-scrolling

One way to fix this is to apply CSS position: fixed;. A possible solution is to leverage the show function and then set the CSS:

                    show: function( event, t ) {
                        t.pointer.css( 'position', 'fixed' );
                    }

amp-pr1271-fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, Tonya! That's a great suggestion to use show. Commit e84e22 does that.

}
);

$( data.pointer.target ).pointer( options ).pointer( 'open' );
}
};
}( jQuery ) );
100 changes: 100 additions & 0 deletions includes/admin/class-amp-admin-pointer.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
<?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() {
$dismissed = explode( ',', strval( get_user_meta( get_current_user_id(), 'dismissed_wp_pointers', true ) ) );
Copy link
Contributor

@hellofromtonya hellofromtonya Jul 19, 2018

Choose a reason for hiding this comment

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

When no pointers have been dismissed, an empty is returned from get_user_meta. In that case, there's no need to run the rest of the code , i.e. strval(), explode(), and in_array(). We can then optimize this code by separating it out.

To keep the method focused on the enqueueing tasks, consider abstracting it into a new method:

	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 );
	}

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.

Hi @hellofromtonya,
Thanks, that's a good suggestion. Self-documenting functions like has_pointer_been_dismissed() help 😄

But I think it only optimizes this if $dismissed is empty...if the user has never dismissed a pointer before.

Once the user dismisses this pointer, $dismissed should at least have the string of this pointer. Unless there was an issue in the POST request in the JS file.

What do you think?

Copy link
Contributor

@hellofromtonya hellofromtonya Jul 20, 2018

Choose a reason for hiding this comment

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

Correct. If the user has never dismissed a pointer, then the user meta does not exist or is empty. In that case, it optimizes the code by bailing out and not running strval(), explode(), or in_array().

Copy link
Contributor

Choose a reason for hiding this comment

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

I also find it expresses the intent more clearly to help us more quickly read and understand what's going on.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kienstra You could name that guard clause method something less passive such as

is_pointer_dismissed()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, commit e60f0 adds is_pointer_dismissed().


// Exit if the pointer has been dismissed.
if ( in_array( self::TEMPLATE_POINTER_ID, $dismissed, true ) ) {
Copy link
Contributor

@hellofromtonya hellofromtonya Jul 19, 2018

Choose a reason for hiding this comment

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

Here, this can now be simplified to this:

		if ( $this->is_pointer_dismissed() ) {
			return;
		}

These changes make it a little bit easier to read and quickly know what's going on in the guard clause.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, this is applied in commit e60f0.

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() ) )
);
}

/**
* 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(
'<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 styles in AMP responses, using "Paired" or "Native" mode.', 'amp' )
),
'position' => array(
'edge' => 'left',
'align' => 'middle',
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.

I think 'edge' => 'left' is probably correct, but the 'align' value might need improvement.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kienstra I think both left and middle works well.

),
),
),
);
}
}
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
93 changes: 93 additions & 0 deletions tests/test-class-amp-admin-pointer.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
<?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 {

/**
* 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();
$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( '<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'] );
}
}