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

Conversation

Projects
None yet
3 participants
@kienstra
Collaborator

kienstra commented Jul 18, 2018

Request For Review

Hi @hellofromtonya,
Thanks for your PR #1274. I'm going to review it next.

It would be great if you could review this. Then, maybe @westonruter could give it another look and merge when it's ready.

  • The admin pointer should appear on checking out this branch and loading /wp-admin.
  • Mainly follows Weston's mockup in Issue 1254.
  • Enqueues the script only if this pointer hasn't been dismissed.
  • Based on this tutorial
  • Copies some of the structure of amp-block-validation.js

Closes #1254

amp-admin-pointer-image

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.
*
* @var string
*/
const TEMPLATE_POINTER_ID = 'amp_template_mode_pointer_10';

This comment has been minimized.

@kienstra

kienstra Jul 19, 2018

Collaborator

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.

function amp_admin_pointer() {
$admin_pointer = new AMP_Admin_Pointer();
$admin_pointer->init();
}

This comment has been minimized.

@kienstra

kienstra Jul 19, 2018

Collaborator

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.

This comment has been minimized.

@hellofromtonya

hellofromtonya Jul 19, 2018

Collaborator

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.

This comment has been minimized.

@kienstra

kienstra Jul 20, 2018

Collaborator

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.

This comment has been minimized.

@hellofromtonya

hellofromtonya Jul 20, 2018

Collaborator

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.

This comment has been minimized.

@kienstra

kienstra Jul 20, 2018

Collaborator

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().

This comment has been minimized.

@hellofromtonya

hellofromtonya Jul 20, 2018

Collaborator

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.

This comment has been minimized.

@kienstra

kienstra Jul 20, 2018

Collaborator

Sounds good, Tonya.

This comment has been minimized.

@westonruter

westonruter Jul 26, 2018

Member

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.

),
'position' => array(
'edge' => 'left',
'align' => 'middle',

This comment has been minimized.

@kienstra

kienstra Jul 19, 2018

Collaborator

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

This comment has been minimized.

@hellofromtonya

hellofromtonya Jul 19, 2018

Collaborator

@kienstra I think both left and middle works well.

@kienstra kienstra requested a review from hellofromtonya Jul 19, 2018

pointer: data.pointer.pointer_id,
action: 'dismiss-wp-pointer'
} );
}

This comment has been minimized.

@hellofromtonya

hellofromtonya Jul 19, 2018

Collaborator

@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

This comment has been minimized.

@kienstra

kienstra Jul 20, 2018

Collaborator

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

* @since 1.0
*/
public function enqueue_pointer() {
$dismissed = explode( ',', strval( get_user_meta( get_current_user_id(), 'dismissed_wp_pointers', true ) ) );

This comment has been minimized.

@hellofromtonya

hellofromtonya Jul 19, 2018

Collaborator

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

This comment has been minimized.

@kienstra

kienstra Jul 20, 2018

Collaborator

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?

This comment has been minimized.

@hellofromtonya

hellofromtonya Jul 20, 2018

Collaborator

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().

This comment has been minimized.

@hellofromtonya

hellofromtonya Jul 20, 2018

Collaborator

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

This comment has been minimized.

@hellofromtonya

hellofromtonya Jul 20, 2018

Collaborator

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

is_pointer_dismissed()

This comment has been minimized.

@kienstra

kienstra Jul 20, 2018

Collaborator

Thanks, commit e60f0 adds is_pointer_dismissed().

$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 ) ) {

This comment has been minimized.

@hellofromtonya

hellofromtonya Jul 19, 2018

Collaborator

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.

This comment has been minimized.

@kienstra

kienstra Jul 20, 2018

Collaborator

Thanks, this is applied in commit e60f0.

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.
@kienstra

This comment has been minimized.

Collaborator

kienstra commented Jul 20, 2018

Hi @hellofromtonya,
Thanks for your great suggestions here. Using show was a great idea.

There are still a few open discussions here. I'm happy to add has_pointer_been_dismissed() if you'd like.

@hellofromtonya

This comment has been minimized.

Collaborator

hellofromtonya commented Jul 20, 2018

I'm happy to add has_pointer_been_dismissed() if you'd like.

@kienstra your code works well. Great job!

I'd vote for abstracting the dismissed guard clause for the following reasons:

  1. It clear communicates intent of what that code is doing.
  2. It's more performant when the user has not dismissed pointers.
  3. It's more readable and quickly understandable.

I suggest naming the method name is_pointer_dismissed() instead of the passive name I originally suggested.

I'll leave it up to you whether you wish to change it or not. I'm approving the PR as your code does work.

@hellofromtonya hellofromtonya requested a review from westonruter Jul 20, 2018

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.
@kienstra

This comment has been minimized.

Collaborator

kienstra commented Jul 20, 2018

Thanks, Applied Suggestion

Hi @hellofromtonya,
Thanks for reviewing this, and adding so much thought into your comments.

Commit e60f0 adds is_pointer_dismissed().

westonruter added some commits Jul 26, 2018

@westonruter westonruter added this to the v1.0 milestone Jul 26, 2018

@westonruter westonruter merged commit 3f09a43 into develop Jul 26, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the add/admin-pointer branch Jul 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment