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

Fix broken tooltips on validation list tables #2149

Merged
merged 4 commits into from
Apr 22, 2019

Conversation

felixarntz
Copy link
Collaborator

@felixarntz felixarntz commented Apr 18, 2019

No longer used. The dependencies do not actually depend on it.

@felixarntz felixarntz requested a review from westonruter April 18, 2019 07:54
@googlebot googlebot added the cla: yes Signed the Google CLA label Apr 18, 2019
Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

@felixarntz I think this is actually being used, though #2148 appears may have broken them.

Admin pointers were being used for the tooltips in the validated URLs post list table:

image

As well as on the validation error taxonomy term list table:

image

In the amp-stories-redux branch, clicking these now does nothing.

They are working in the develop branch however.

Ultimately, as identified in #1479 we need to eliminate the use of pointers altogether for this purpose.

@felixarntz Could you use this PR to fix the pointers broken in #2148?

@felixarntz felixarntz changed the title Remove now unused tooltips script. Fix broken tooltips on validation list tables Apr 22, 2019
@felixarntz felixarntz added this to the v1.1.1 milestone Apr 22, 2019
@westonruter westonruter removed this from the v1.1.1 milestone Apr 22, 2019
Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

@felixarntz I don't think these changes are right. I think it is this code that should be restored:

add_action( 'admin_enqueue_scripts', array( $this, 'register_tooltips' ) );

/**
* Registers style and script for tooltips.
*
* @since 1.0
*/
public function register_tooltips() {
wp_register_style(
self::TOOLTIP_SLUG,
amp_get_asset_url( 'css/' . self::TOOLTIP_SLUG . '.css' ),
array( 'wp-pointer' ),
AMP__VERSION
);
wp_register_script(
self::TOOLTIP_SLUG,
amp_get_asset_url( 'js/' . self::TOOLTIP_SLUG . '.js' ),
array( 'jquery', 'wp-pointer' ),
AMP__VERSION,
true
);
}

This code is in develop and 1.1 so no need to milestone the changes for 1.1.1.

Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

Actually, since we have to revisit this anyway in #1479 it seems fine to just go ahead and merge this now.

@westonruter westonruter merged commit 5b1e3cd into amp-stories-redux Apr 22, 2019
@westonruter westonruter deleted the add/amp-stories-admin-pointer branch April 22, 2019 05:58
@westonruter westonruter added this to the v1.2 milestone May 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Signed the Google CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants