Add buttons to copy error details to clipboard by johnwatkins0 · Pull Request #5500 · ampproject/amp-wp

johnwatkins0 · 2020-10-12T20:09:24Z

Summary

This modifies the single AMP Validated URL screen by making it easier to copy error details to the user's clipboard in pretty-printed JSON format. Users can take action in two ways:

By clicking this "Copy to clipboard" button in the actions for a single error (a single JSON object is copied):

By clicking this "Copy error details to clipboard" button in the bulk actions row (an array of JSON objects is copied):

These buttons are to be cleaned up as part of #5431.

QA steps are in #5209

Fixes #5209

Checklist

My pull request is addressing an open issue (please create one otherwise).
My code is tested and passes existing tests.
My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

github-actions · 2020-10-12T22:03:50Z

Plugin builds for 47fc7be are ready 🛎️!

Download development build
Download production build

johnwatkins0 · 2020-10-12T22:11:29Z

-				row.classList.toggle( 'new', ! markingAsReviewed );
-				addBeforeUnloadPrompt();
-			}
+		getURLValidationTableRows( { checkedOnly: true } ).forEach( ( row ) => {


This is a tangential change. I found myself copying pasting this code (which is from another PR I worked on recently) and instead created the getURLValidationTableRows helper function.

pierlon · 2020-10-19T21:28:41Z

@@ -0,0 +1,61 @@
+/**


Alternatively you could use clipboard.js (which comes bundled with WordPress), but this approach is also fine 👍.

Good suggestion. I didn't think of this. I've refactored out the wheel-reinvention in 61e571a

pierlon · 2020-10-19T21:44:24Z

 				esc_html__( 'Details', 'amp' )
 			);
+
+			$json           = json_decode( $term->description, true );


So for a single error the JSON would look like:

{ "code": "DISALLOWED_TAG", "node_attributes": { "src": "https:\/\/wordpressdev.lndo.site\/content\/themes\/twentytwentyone\/assets\/js\/primary-navigation.js?ver=__normalized__", "id": "twenty-twenty-one-primary-navigation-script-js" }, "node_name": "script", "node_type": 1, "parent_name": "body", "type": "js_error", "status": "Removed" }

For the node_type field, I'm wondering if we should get the XML constant name from the value and use that instead. By itself, the constant value wouldn't be of much use and would require the person having to lookup the constants themselves. For reference, here's the list:

We could reflect the XMLReader class and obtain the constants from there

Done in fe35801

pierlon · 2020-10-19T21:49:44Z

+	}
+
+	const value = getURLValidationTableRows( { checkedOnly: true } ).map( ( row ) => {
+		const copyButton = row.querySelector( '.single-url-detail-copy' );


I noticed that if the user changes any of the row's values (without updating the post), the JSON is not updated. For example, updating a validation error to be kept and then copying the JSON for the row does not reflect the updated field value.

Fixed in c4e1a64

johnwatkins0 · 2020-10-21T14:58:58Z

Thanks for the suggestions, @pierlon. I've updated and am re-requesting now.

CLAassistant · 2020-10-22T22:05:37Z

All committers have signed the CLA.

pierlon · 2020-10-27T04:48:43Z

The only issue I have with this currently is that the "Copy error details to clipboard" button nor the "Copy to clipboard" buttons stay focused when clicked. Also, should we provide a visual feedback to indicate that the JSON has successfully been copied?

johnwatkins0 · 2020-10-27T16:04:28Z

The only issue I have with this currently is that the "Copy error details to clipboard" button nor the "Copy to clipboard" buttons stay focused when clicked. Also, should we provide a visual feedback to indicate that the JSON has successfully been copied?

Good call, I will update.

johnwatkins0 · 2020-10-28T15:01:03Z

@pierlon I've addressed your suggestions in ddf5ec4

The button is focused on success.
The button text is updated for four seconds. (I referred to this Gutenberg component for inspiration.)

Let me know what you think.

pierlon · 2020-10-28T18:33:56Z

+ * @param {HTMLElement} event.trigger The element triggering the event.
+ */
+function onSuccess( { trigger } ) {
+	trigger.focus();


Minor bug:
Clicking on the button while it's showing the updated text permanently sets it to said text:

Yes, looking at my code, that bug makes perfect sense 🤦 Fixing.

Thanks for the catch, @pierlon. Fixed in 47fc7be

pierlon

Well done!

Co-authored-by: Weston Ruter <westonruter@google.com>

westonruter · 2020-10-29T18:55:53Z

+
+		$json['status'] = (bool) ( (int) $term->term_group & self::ACCEPTED_VALIDATION_ERROR_BIT_MASK ) ? __( 'Removed', 'amp' ) : __( 'Kept', 'amp' );
+
+		return wp_json_encode( $json, JSON_PRETTY_PRINT );


Since the slashes aren't needed in this context (and they impair readability):

Suggested change

return wp_json_encode( $json, JSON_PRETTY_PRINT );

return wp_json_encode( $json, JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES );

I suppose this is not needed if the JSON is re-parsed and re-serialized. If that's the case, then JSON_PRETTY_PRINT isn't needed either.

Updated in c83dbb8 and 54ecc46

Co-authored-by: Weston Ruter <westonruter@google.com>

…iewed field Co-authored-by: Weston Ruter <westonruter@google.com>

westonruter · 2020-10-29T19:14:31Z

@johnwatkins0 Do you want to rebase this onto develop? That should fix the build errors.

…oard-friendly-errors

Co-authored-by: Weston Ruter <westonruter@google.com>

johnwatkins0 · 2020-10-29T19:24:07Z

@westonruter I've addressed all feedback and merged in the latest develop, but as you can see, there is still a composer issue in the actions building the zip files. I saw this in another PR earlier.

…209-clipboard-friendly-errors * 'develop' of github.com:ampproject/amp-wp: (174 commits) Temporarily force of Composer v1 in CI (#5543) Fix typo in Optimizer readme (#5535) Include file.php separately from class-wp-upgrader.php (#5539) Add PhpIncludeInspection annotation Add PhpUnusedParameterInspection annotation Capture the original block render_callbacks to detect originating plugin Fix Gutenberg 9.2.1 compat due to gutenberg_current_parsed_block_tracking() Ensure full coverage for test_get_plugin_dir Skip Facebook tests earlier when external-http; update skip message Fix CSSList splicing and eliminate reliance on buggy replace/splice methods Add more removed rules that cause splice issue Add charset rule to allowed_at_rules_retained test data Ensure -moz-document rule is removed after replaced Skip FB test once testing external-http suite Add test for PluginRegistry::get_plugin_dir() Ensure css_list is sanitized after -moz-document transformation Transform -moz-document at-rule Firefox hack into supports hack Fix lint error Manually bootstrap WP CLI Skip test if Facebook oEmbed endpoint is down ...

…mpproject/amp-wp into feature/5209-clipboard-friendly-errors * 'feature/5209-clipboard-friendly-errors' of github.com:ampproject/amp-wp: Remove unnecessary aria-label Parse and re-stringify JSON data to prettify No need to pretty-print error details on the backend

westonruter · 2020-10-29T19:28:02Z

there is still a composer issue in the actions building the zip files. I saw this in another PR earlier.

Isn't this addressed by #5543?

westonruter

Love this functionality 👍

pierlon · 2020-10-29T19:34:29Z

there is still a composer issue in the actions building the zip files. I saw this in another PR earlier.

Isn't this addressed by #5543?

@johnwatkins0 is referring to the current GHA workflow used to build the plugin; it's also now using composer v2 which is causing the jobs to fail.

westonruter · 2020-10-29T19:44:09Z

I've run the tests locally and they pass (aside from the oEmbed handlers that . So I'm going ahead and merging.

Add buttons to copy error details to clipboard

0b8c793

google-cla Bot added the cla: yes Signed the Google CLA label Oct 12, 2020

johnwatkins0 added 3 commits October 12, 2020 15:35

Move clipboard functions into own module and prettyprint JSON

b2b5c14

Resolve failing test

3cfad55

Add tests for clipboard-friendly error button

f73e574

johnwatkins0 marked this pull request as ready for review October 12, 2020 22:00

johnwatkins0 requested review from pierlon and westonruter October 12, 2020 22:08

johnwatkins0 commented Oct 12, 2020

View reviewed changes

johnwatkins0 self-assigned this Oct 12, 2020

westonruter added the WS:Core Work stream for Plugin core label Oct 15, 2020

pierlon suggested changes Oct 19, 2020

View reviewed changes

johnwatkins0 added 3 commits October 21, 2020 09:15

Convert error node_type to constant name in JSON

fe35801

Update markup status in JSON data in case it has changed

c4e1a64

Use clipboard.js package for copying to clipboard

61e571a

johnwatkins0 requested a review from pierlon October 21, 2020 14:59

johnwatkins0 added 2 commits October 21, 2020 10:25

Disable eslint rule per line

08d9237

Remove unneeded helper function

0153b68

Copy to clipboard: restore focus state and show 'copied' text

ddf5ec4

pierlon suggested changes Oct 28, 2020

View reviewed changes

Copy to clipboard: handle case where clicks button again immediately

47fc7be

pierlon approved these changes Oct 28, 2020

View reviewed changes

westonruter reviewed Oct 29, 2020

View reviewed changes

Comment thread includes/validation/class-amp-validated-url-post-type.php Resolved

Make 'copy to clipboard' text consistent

7e1d5a9

Co-authored-by: Weston Ruter <westonruter@google.com>

westonruter requested changes Oct 29, 2020

View reviewed changes

johnwatkins0 and others added 2 commits October 29, 2020 14:09

Clipboard-friendly errors: use status from DB instead of from UI

4bae05c

Co-authored-by: Weston Ruter <westonruter@google.com>

Clipboard-friendly errors: convert status to removed bool and add rev…

d2382e8

…iewed field Co-authored-by: Weston Ruter <westonruter@google.com>

johnwatkins0 and others added 4 commits October 29, 2020 14:15

No need to pretty-print error details on the backend

c83dbb8

Merge remote-tracking branch 'origin/develop' into feature/5209-clipb…

e7dbb5d

…oard-friendly-errors

Parse and re-stringify JSON data to prettify

54ecc46

Remove unnecessary aria-label

23fbfed

Co-authored-by: Weston Ruter <westonruter@google.com>

westonruter added 2 commits October 29, 2020 12:24

westonruter approved these changes Oct 29, 2020

View reviewed changes

westonruter added this to the v2.1 milestone Oct 29, 2020

Fix test_get_error_details_json

f6bb8ad

westonruter merged commit 1160864 into develop Oct 29, 2020

westonruter deleted the feature/5209-clipboard-friendly-errors branch October 29, 2020 19:44

westonruter mentioned this pull request Feb 1, 2021

Validation errors JSON data copied to clipboard lacks sources #5825

Closed

westonruter mentioned this pull request Dec 22, 2021

Facilitate users to share validation issues with 3rd party theme/plugin developers #6807

Closed


		$json['status'] = (bool) ( (int) $term->term_group & self::ACCEPTED_VALIDATION_ERROR_BIT_MASK ) ? __( 'Removed', 'amp' ) : __( 'Kept', 'amp' );

		return wp_json_encode( $json, JSON_PRETTY_PRINT );

Conversation

johnwatkins0 commented Oct 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

github-actions Bot commented Oct 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnwatkins0 commented Oct 21, 2020

Uh oh!

CLAassistant commented Oct 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pierlon commented Oct 27, 2020

Uh oh!

johnwatkins0 commented Oct 27, 2020

Uh oh!

johnwatkins0 commented Oct 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pierlon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

westonruter commented Oct 29, 2020

Uh oh!

johnwatkins0 commented Oct 29, 2020

Uh oh!

westonruter commented Oct 29, 2020

Uh oh!

westonruter left a comment

Choose a reason for hiding this comment

Uh oh!

pierlon commented Oct 29, 2020

Uh oh!

westonruter commented Oct 29, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

johnwatkins0 commented Oct 12, 2020 •

edited

Loading

github-actions Bot commented Oct 12, 2020 •

edited

Loading

CLAassistant commented Oct 22, 2020 •

edited

Loading