Add AMP plugin editor sidebar #5589

johnwatkins0 · 2020-11-10T20:56:44Z

Summary

On the editor screen, this moves all info about validation errors into a new editor sidebar.

The sidebar shows for users with dev tools turned on
It shows if the WP version is 5.3 or above
Error details include key information:

Note: The newBlockSources PHP class is used to get the plugin or theme name responsible for a block (if the block is registered in PHP). It depends on a filter that was introduced in WP 5.5, though, so in older versions of WP the "Source" line falls back to the registered block name (e.g., contact-form/form) similar to how blocks are currently shown on the URL validation screen. Like this:

The "View details" link shown above goes to the URL validation screen in a new tab, opens the error, and scrolls to it.
If any errors are kept, the toolbar icon changes and this notice appears:

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

…b.com/ampproject/amp-wp into feature/3821-block-validation-sidebar

…-validation-sidebar

johnwatkins0 · 2020-11-20T18:20:21Z

This is not ready for review, but I un-Drafted it to trigger the zip build.

pierlon · 2020-11-20T20:10:05Z

@johnwatkins0 the "Lint: JS" job is failing so zip builds won't occur.

…-validation-sidebar

github-actions · 2020-11-24T03:32:59Z

Plugin builds for 96c1a96 are ready 🛎️!

Download development build
Download production build

Co-authored-by: Pierre Gordon <[email protected]>

…b.com/ampproject/amp-wp into feature/3821-block-validation-sidebar

Co-authored-by: Pierre Gordon <[email protected]>

johnwatkins0 · 2020-12-28T17:03:56Z

I wonder if the user will be confused when certain types of block icons are paired with the error icons. For example, I at first was a bit confused when I saw this (there was a JS error in the Custom HTML block):

I think we can handle this better in the follow-up issue to adding filters and updating the design. @jwold is currently working on the visual design for that.

Also, I forsee the orange outline being a potential problem if the theme background were to be of a similar color. For example, when I have the Twenty Twenty-One theme activated with the background color #ba6a50 set I see:

This is indeed problematic, but I'm not sure of the best solution given that the AMP plugin doesn't know what types of styles the theme might be applying to the editor. Similar issues can occur with core styles also. So I think we can probably leave this one alone.

There are 2 blocks outlined in the image above but they are almost impossible to see.

I was expecting the AMP toolbar button on the block to open the sidebar to the validation error specific to the block. Would that be possible?

Done in 1d6a0a3

When I inserted a Custom HTML block with the following contents and saved the post the editor crashed:
<?php

Fixed in b1a6a11

pierlon

This works well so far. The only quirk I found is with the AMP toolbar button, where it can now be used to toggle the error panel in the sidebar. Is that intended? I'd imagine it should only be able to open the error panel.

amp_toolbar_button_click.mp4

johnwatkins0 · 2020-12-29T22:56:13Z

The only quirk I found is with the AMP toolbar button, where it can now be used to toggle the error panel in the sidebar. Is that intended?

Yes, I see what you mean. Updated in 29cf878. The button will only open the error, not close it.

pierlon

Nicely done! 👌

…821-block-validation-sidebar * 'develop' of github.com:ampproject/amp-wp: (95 commits) Simplify logic to ensure -1 <= limit_per_type <= PHP_INT_MAX Ensure amp_url_validation_limit_per_type filter returns int Mark URLValidationProvider and ScannableURLProvider as internal Guard against empty post ID causing global post to be returned by get_post() Make RecurringBackgroundTask::schedule_event() final Fix ability for BackgroundTaskDeactivator to unschedule hooks Reduce URLValidationCron frequency to daily Remove obsolete test assertion Make CronBasedBackgroundTask non-Conditional Update service name for SavePostValidationEvent and add to PHPSTORM_META Add missing $args to wp_next_scheduled() call after 1419ef4 Switch to google-github-actions/setup-gcloud action Only run PHPStan if there is a modified PHP file Run composer update Ensure alphabetical order for patches in composer.json Use PHPStan ignore for 'cannot cast array to int' Reorder composer.json fields Update param type for phpstan Remove obsolete amp_is_canonical() examples Reorder composer.json fields ...

westonruter · 2020-12-30T05:45:30Z

Minor thing: When the AMP plugin sidebar is not pinned, I'm seeing the icon not positioned optimally:

westonruter · 2020-12-30T05:48:39Z

Fixed in ec509b8:

…wport

westonruter · 2020-12-30T06:16:35Z

assets/src/block-validation/error/error-panel-title.js

-					<ErrorTypeIcon type={ type } />
-				</div>
+				{ type && (
+					<div className={ `amp-error__error-type-icon amp-error__error-type-icon--${ type?.replace( /_/g, '-' ) }` }>


Optional chaining is unnecessary here since type && ensures it is defined, no?

Suggested change

<div className={ `amp-error__error-type-icon amp-error__error-type-icon--${ type?.replace( /_/g, '-' ) }` }>

<div className={ `amp-error__error-type-icon amp-error__error-type-icon--${ type.replace( /_/g, '-' ) }` }>

westonruter · 2020-12-30T06:18:54Z

assets/src/block-validation/error/index.js

@@ -70,15 +69,14 @@ export function Error( { clientId, status, term_id: termId, ...props } ) {
 							{ __( 'Select block', 'amp' ) }
 						</Button>
 					) }
-					<a
+					<ExternalLink
 						href={ detailsUrl.href }
 						target="_blank"


Instead of _blank we may want this to be a consistent name like validated-url-post-${ postId } so that each of the View Details links will be routed to the same tab.

westonruter · 2020-12-30T06:19:34Z

assets/src/block-validation/sidebar.js

@@ -100,10 +99,9 @@ export function Sidebar() {

 						<p>
 							{ reviewLink && (
-								<a href={ reviewLink } className="amp-sidebar__review-link" target="_blank" rel="noreferrer">
+								<ExternalLink href={ reviewLink } className="amp-sidebar__review-link">


Adding a target per https://github.com/ampproject/amp-wp/pull/5589/files/76171256101db27f82839d3832b3b614bac44534..66666f081bae2e6f2ecde9364cecdfa3caae635b?w=1#r549947592 above may make sense.

Co-authored-by: Pierre Gordon <[email protected]>

johnwatkins0 added 11 commits November 10, 2020 14:54

AMP plugin sidebar

127a7fa

Fix ampBroken logic

b3e3058

Change new validation errors heading text

3a69c9a

AMP plugin sidebar

cedf6f5

Fix ampBroken logic

ee94295

Change new validation errors heading text

e4613b4

Sidebar: update error display logic and add new styles

43ed5b2

Merge branch 'feature/3821-block-validation-sidebar' of https://githu…

a52d018

…b.com/ampproject/amp-wp into feature/3821-block-validation-sidebar

Pass initially open term ID into URL validation screen

c827669

Scroll into view

987ab40

Merge remote-tracking branch 'origin/develop' into feature/3821-block…

15337e7

…-validation-sidebar

johnwatkins0 marked this pull request as ready for review November 20, 2020 18:19

johnwatkins0 added 9 commits November 21, 2020 10:43

Update error content text

5ef519d

Icon style updates

eea577c

Create block sources service class

df4ba1c

Integrate block sources info and add tests

68f0e1d

Fix JS lint issues

55d3577

Merge remote-tracking branch 'origin/develop' into feature/3821-block…

710df98

…-validation-sidebar

Add more Jest coverage for error component

71106ec

PHPCS

f5040f4

PHPStan fix

88060a9

johnwatkins0 added 3 commits November 23, 2020 21:40

Fix PHPUnit test

4096fff

Put AMPBlockValidation into index.js

9a056bf

PHPUnit test fix

28cd8a3

johnwatkins0 force-pushed the feature/3821-block-validation-sidebar branch from 9386754 to 28cd8a3 Compare November 24, 2020 04:02

johnwatkins0 added 2 commits November 23, 2020 22:32

Set screen in validation manager test setup to ensure dependencies load

3dd14fa

Fix dependencies array in test

28f4deb

johnwatkins0 and others added 7 commits December 28, 2020 10:21

Fix import comments

6a3df3f

Co-authored-by: Pierre Gordon <[email protected]>

Minor comment fixes

af1dc4d

Co-authored-by: Pierre Gordon <[email protected]>

Add Services::has method

e3721ec

Merge branch 'feature/3821-block-validation-sidebar' of https://githu…

473f6ab

…b.com/ampproject/amp-wp into feature/3821-block-validation-sidebar

Apply suggestions from code review

31278f2

Co-authored-by: Pierre Gordon <[email protected]>

Handle case where error has no type

b1a6a11

Open error when its block icon is clicked

1d6a0a3

johnwatkins0 requested a review from pierlon December 28, 2020 17:04

pierlon suggested changes Dec 29, 2020

View reviewed changes

AMP block toolbar button: prevent closing error if already opened

29cf878

johnwatkins0 requested a review from pierlon December 29, 2020 22:56

pierlon approved these changes Dec 30, 2020

View reviewed changes

Add missing class to amp-toolbar-icon

ec509b8

westonruter added 2 commits December 29, 2020 22:08

Expand all validation errors associatd with a block and ensure in vie…

8f5fbb8

…wport

Prevent invalid-block-outline from causing block to be unclickable

c1b5b86

westonruter reviewed Dec 30, 2020

View reviewed changes

Further refine messaging related to validation issues

96c1a96

Co-authored-by: Pierre Gordon <[email protected]>

westonruter approved these changes Dec 30, 2020

View reviewed changes

westonruter merged commit 8eb49f1 into develop Dec 30, 2020

westonruter deleted the feature/3821-block-validation-sidebar branch December 30, 2020 06:30

westonruter mentioned this pull request Jan 5, 2021

Add explainer to warning notice when validation errors are displayed #5304

Closed

westonruter mentioned this pull request Mar 4, 2021

AMP preview button seems to show duplicate logo when AMP sidebar is pinned #5943

Closed

westonruter added this to the v2.1 milestone Mar 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AMP plugin editor sidebar #5589

Add AMP plugin editor sidebar #5589

johnwatkins0 commented Nov 10, 2020 •

edited

Loading

johnwatkins0 commented Nov 20, 2020

pierlon commented Nov 20, 2020

github-actions bot commented Nov 24, 2020 •

edited

Loading

johnwatkins0 commented Dec 28, 2020 •

edited

Loading

pierlon left a comment

johnwatkins0 commented Dec 29, 2020

pierlon left a comment

westonruter commented Dec 30, 2020

westonruter commented Dec 30, 2020

westonruter Dec 30, 2020

westonruter Dec 30, 2020

westonruter Dec 30, 2020

	<div className={ `amp-error__error-type-icon amp-error__error-type-icon--${ type?.replace( /_/g, '-' ) }` }>
	<div className={ `amp-error__error-type-icon amp-error__error-type-icon--${ type.replace( /_/g, '-' ) }` }>

Add AMP plugin editor sidebar #5589

Add AMP plugin editor sidebar #5589

Conversation

johnwatkins0 commented Nov 10, 2020 • edited Loading

Summary

Checklist

johnwatkins0 commented Nov 20, 2020

pierlon commented Nov 20, 2020

github-actions bot commented Nov 24, 2020 • edited Loading

johnwatkins0 commented Dec 28, 2020 • edited Loading

pierlon left a comment

Choose a reason for hiding this comment

johnwatkins0 commented Dec 29, 2020

pierlon left a comment

Choose a reason for hiding this comment

westonruter commented Dec 30, 2020

westonruter commented Dec 30, 2020

westonruter Dec 30, 2020

Choose a reason for hiding this comment

westonruter Dec 30, 2020

Choose a reason for hiding this comment

westonruter Dec 30, 2020

Choose a reason for hiding this comment

johnwatkins0 commented Nov 10, 2020 •

edited

Loading

github-actions bot commented Nov 24, 2020 •

edited

Loading

johnwatkins0 commented Dec 28, 2020 •

edited

Loading