AMP validation - add pointer tooltips for status columns #1448
Conversation
johnwatkins0
changed the title
johnwatkins0
changed the title
|
Thanks, @johnwatkins0! If it's alright, I'll defer to Weston for the code review. |
| 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' ), |
There was a problem hiding this comment.
It looks like wp-dom-ready might only be available in Gutenberg (or at least it's merge).
There was a problem hiding this comment.
It should not be made a script dependency. It is a module dependency.
* 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' ), |
There was a problem hiding this comment.
It should not be made a script dependency. It is a module dependency.
|
|
||
| // WIP Pointer function | ||
| function sourcesPointer() { | ||
| jQuery( '.tooltip' ).on( 'hover', function() { |
There was a problem hiding this comment.
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.
| 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' ), |
There was a problem hiding this comment.
Placeholder title?
Should be using esc_html__().
| '%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' ) |
There was a problem hiding this comment.
Should be using esc_html__().
| '%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' ) |
There was a problem hiding this comment.
Should be using esc_html__().
| '%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' ) |
There was a problem hiding this comment.
Should be using esc_html__().
| @@ -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>', | |||
There was a problem hiding this comment.
The div needs tabindex=0 so that it can be focused with the keyboard.
| } | ||
|
|
||
| .wp-pointer--tooltip { | ||
| transform: translateX(-52px); |
There was a problem hiding this comment.
This transform seems to be causing jumpiness.
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
Tooltipcomponent from the@wordpress/componentspackage 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.