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

AMP validation - add pointer tooltips for status columns #1448

Merged
merged 5 commits into from Sep 21, 2018

Conversation

@johnwatkins0
Copy link
Contributor

@johnwatkins0 johnwatkins0 commented Sep 20, 2018

This partially resolves #1400 by adding a tooltip to the headings of "Status" columns on AMP validation screens. It's not fully resolved because (1) we'll need to put in the final text, and (2) the styling and positioning of the tooltips could potentially use a little fine-tuning. Further, the tooltips could easily be created with the Tooltip component from the @wordpress/components package where Gutenberg-derived assets are available -- but I didn't go this route now because that tooltip looks much different from the one that had been decided on for this task.

Edit: Added the tooltip to the "Details" column as well. There's a little bit of conflicting information on this in the tickets/comps. It can easily be removed.

@johnwatkins0 johnwatkins0 changed the title AMP validation - add pointer tooltips for status columns [WIP] AMP validation - add pointer tooltips for status columns Sep 20, 2018
@johnwatkins0 johnwatkins0 changed the title [WIP] AMP validation - add pointer tooltips for status columns AMP validation - add pointer tooltips for status columns Sep 20, 2018
@kienstra
Copy link
Contributor

@kienstra kienstra commented Sep 20, 2018

Thanks, @johnwatkins0! If it's alright, I'll defer to Weston for the code review.

Loading

AMP__VERSION
);
wp_enqueue_script(
'amp-validation-error-detail-toggle',
amp_get_asset_url( 'js/amp-validation-error-detail-toggle-compiled.js' ),
array(),
array( 'wp-dom-ready', 'amp-validation-tooltips' ),
Copy link
Contributor

@kienstra kienstra Sep 20, 2018

Choose a reason for hiding this comment

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

It looks like wp-dom-ready might only be available in Gutenberg (or at least it's merge).

Loading

Copy link
Member

@westonruter westonruter Sep 21, 2018

Choose a reason for hiding this comment

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

It should not be made a script dependency. It is a module dependency.

Loading

* Ensure tooltip is keyboard accessible.
* Fix importing of domReady module.
* Add escaping translation functions.
* Remove placeholder tooltip titles.
AMP__VERSION
);
wp_enqueue_script(
'amp-validation-error-detail-toggle',
amp_get_asset_url( 'js/amp-validation-error-detail-toggle-compiled.js' ),
array(),
array( 'wp-dom-ready', 'amp-validation-tooltips' ),
Copy link
Member

@westonruter westonruter Sep 21, 2018

Choose a reason for hiding this comment

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

It should not be made a script dependency. It is a module dependency.

Loading


// WIP Pointer function
function sourcesPointer() {
jQuery( '.tooltip' ).on( 'hover', function() {
Copy link
Member

@westonruter westonruter Sep 21, 2018

Choose a reason for hiding this comment

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

I don't think that hover works. The tooltip really needs to not get in the way. It's too easy to trigger it. The problem could be solved if we switch to showing the tooltip on click rather than on hover. This would also make it keyboard accessible.

Loading

AMP_Validation_Error_Taxonomy::ERROR_STATUS => sprintf(
'%s<div class="tooltip dashicons dashicons-editor-help"><h3>%s</h3><p>%s</p></div>',
esc_html__( 'Status', 'amp' ),
__( 'Statuses tooltip title', 'amp' ),
Copy link
Member

@westonruter westonruter Sep 21, 2018

Choose a reason for hiding this comment

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

Placeholder title?

Should be using esc_html__().

Loading

'%s<div class="tooltip dashicons dashicons-editor-help"><h3>%s</h3><p>%s</p></div>',
esc_html__( 'Status', 'amp' ),
__( 'Statuses tooltip title', 'amp' ),
__( 'An accepted validation error is one that will not block a URL from being served as AMP; the validation error will be sanitized, normally resulting in the offending markup being stripped from the response to ensure AMP validity.', 'amp' )
Copy link
Member

@westonruter westonruter Sep 21, 2018

Choose a reason for hiding this comment

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

Should be using esc_html__().

Loading

'%s<div class="tooltip dashicons dashicons-editor-help"><h3>%s</h3><p>%s</p></div>',
__( 'Status', 'amp' ),
__( 'Statuses tooltip title', 'amp' ),
__( 'An accepted validation error is one that will not block a URL from being served as AMP; the validation error will be sanitized, normally resulting in the offending markup being stripped from the response to ensure AMP validity.', 'amp' )
Copy link
Member

@westonruter westonruter Sep 21, 2018

Choose a reason for hiding this comment

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

Should be using esc_html__().

Loading

'%s<div class="tooltip dashicons dashicons-editor-help"><h3>%s</h3><p>%s</p></div>',
__( 'Details', 'amp' ),
__( 'Details tooltip title', 'amp' ),
__( 'An accepted validation error is one that will not block a URL from being served as AMP; the validation error will be sanitized, normally resulting in the offending markup being stripped from the response to ensure AMP validity.', 'amp' )
Copy link
Member

@westonruter westonruter Sep 21, 2018

Choose a reason for hiding this comment

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

Should be using esc_html__().

Loading

@@ -702,7 +700,12 @@ public static function add_post_columns( $columns ) {
$columns = array_merge(
$columns,
array(
AMP_Validation_Error_Taxonomy::ERROR_STATUS => sprintf( '%s<span class="dashicons dashicons-editor-help"></span>', esc_html__( 'Status', 'amp' ) ), // @todo Create actual tooltip.
AMP_Validation_Error_Taxonomy::ERROR_STATUS => sprintf(
'%s<div class="tooltip dashicons dashicons-editor-help"><h3>%s</h3><p>%s</p></div>',
Copy link
Member

@westonruter westonruter Sep 21, 2018

Choose a reason for hiding this comment

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

The div needs tabindex=0 so that it can be focused with the keyboard.

Loading

}

.wp-pointer--tooltip {
transform: translateX(-52px);
Copy link
Member

@westonruter westonruter Sep 21, 2018

Choose a reason for hiding this comment

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

This transform seems to be causing jumpiness.

Loading

@westonruter westonruter merged commit 4c00595 into develop Sep 21, 2018
2 checks passed
Loading
@westonruter westonruter deleted the add/1400-tooltips branch Sep 21, 2018
@westonruter westonruter added this to the v1.0 milestone Sep 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants