Update editor sidebar validation notifications #5929

delawski · 2021-02-25T17:56:01Z

Summary

Fixes #5304
Relates to #5741

This PR updates the look and feel of the AMP sidebar, most notably the validation notifications area.

List of collapsed errors:

Expanded error with a long message:

Here are the most important changes included in this PR:

Extract the logic that determines the messaging in the sidebar from the <Sidebar /> component into the new amp-validation-status components (<AMPValidationStatusNotification /> and <AMPRevalidateNotification />).
Introduce a generic <SidebarNotification /> component (along with a dedicated container component).
Make the error title in the panel title more compact when collapsed and more descriptive when expanded.
Use the text-overflow: ellipsis property for error title when collapsed.
Remove the now redundant title from the error content area (title in full is already part of the panel title).
Add an icon to the block type component in the error content area.
Mark only the "kept" errors with a red left border and remove the exclamation mark from the error panel title.
Mark new errors with a light grey background and bold font.
Use a white background and a regular font-weight for the reviewed errors.
Clean up the CSS and use alphabetical ordering for properties.
Extract amp-error-related CSS to a separate stylesheet.
Use classname-based CSS selectors for the errors list and list items.
Don't escape error message title so that HTML tags like <code> are properly rendered.

The new components are meant to be reused in the Pre-Publish Dialogue and in the general Sidebar, in the AMP section.

Checklist

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

codecov · 2021-02-25T18:00:25Z

Codecov Report

Merging #5929 (a1e079a) into develop (3d4e967) will increase coverage by 0.12%.
The diff coverage is 85.29%.

❗ Current head a1e079a differs from pull request most recent head 17ef326. Consider uploading reports for the commit 17ef326 to get more accurate results

@@              Coverage Diff              @@
##             develop    #5929      +/-   ##
=============================================
+ Coverage      75.28%   75.41%   +0.12%     
- Complexity      5722     5735      +13     
=============================================
  Files            214      218       +4     
  Lines          17283    17362      +79     
=============================================
+ Hits           13012    13094      +82     
+ Misses          4271     4268       -3

Flag	Coverage Δ	Complexity Δ
javascript	`80.05% <89.18%> (+0.60%)`	`0.00 <0.00> (ø)`
php	`75.22% <80.64%> (+0.09%)`	`0.00 <13.00> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
...ts/src/block-validation/error/error-panel-title.js	`100.00% <ø> (ø)`	`0.00 <0.00> (ø)`
...udes/sanitizers/class-amp-core-theme-sanitizer.php	`35.87% <0.00%> (-0.26%)`	`229.00 <1.00> (+1.00)`	⬇️
includes/sanitizers/class-amp-meta-sanitizer.php	`79.79% <0.00%> (ø)`	`35.00 <0.00> (ø)`
assets/src/block-validation/store.js	`94.73% <66.66%> (-5.27%)`	`0.00 <0.00> (ø)`
...n/amp-validation-status/revalidate-notification.js	`80.00% <80.00%> (ø)`	`0.00 <0.00> (?)`
src/Transformer/DetermineHeroImages.php	`91.04% <90.19%> (+38.66%)`	`25.00 <12.00> (+12.00)`
...k-validation/use-validation-error-state-updates.js	`85.07% <92.85%> (+0.56%)`	`0.00 <0.00> (ø)`
...ck-editor/components/sidebar-notification/index.js	`100.00% <100.00%> (ø)`	`0.00 <0.00> (?)`
...ation/amp-validation-status/status-notification.js	`100.00% <100.00%> (ø)`	`0.00 <0.00> (?)`
assets/src/block-validation/error/error-content.js	`93.54% <100.00%> (ø)`	`0.00 <0.00> (ø)`
... and 11 more

github-actions · 2021-02-25T18:01:10Z

Plugin builds for 17ef326 are ready 🛎️!

Download development build
Download production build

westonruter · 2021-03-02T03:27:24Z

@delawski FYI: There's a conflict now with develop.

delawski · 2021-03-02T11:35:53Z

@delawski FYI: There's a conflict now with develop.

Thanks for the heads-up. I'll take care of the conflicts when working on the few updates that are still on my todo list for this PR.

jwold · 2021-03-02T14:57:40Z

@delawski for error handling what is an example of what an error might say?
Assuming you then need a UI displaying that error. Correct?

delawski · 2021-03-04T17:57:08Z

For error handling what is an example of what an error might say?

Here's an example of an error message returned by the backend:

Assuming you then need a UI displaying that error. Correct?

Correct.

# Conflicts: # assets/src/block-validation/sidebar.js

On the build step, I was getting the following error thrown by the `block-validation/icon.js`: ``` Property value of ObjectProperty expected node to be of a type ["Expression","PatternLike"] but instead got "JSXExpressionContainer" ``` Cleaning up the `amp-logo-icon.svg` markup and passing it through svgomg fixed the issue.

delawski · 2021-03-08T17:59:07Z

@westonruter @jwold With 543ab48 and c37125e a loading state and an API errors handling have been added to the sidebar notification area:

sidebar-api-error.mov

The error message displayed in the notification is whatever the REST endpoint returns. This might be improved over time. However, since such API-level errors are rather rare I wouldn't worry about that too much at this point.

jwold · 2021-03-09T21:48:01Z

This is perfect for now. Great job @delawski !

…304-validation-notifications * 'develop' of github.com:ampproject/amp-wp: (77 commits) Summarize cleaned markdown file count Indicate WP_CLI::error() among earlyTerminatingMethodCalls Revert change to WORDPRESS_DB_USER Remove sizereport.config.js Rename env file Move duplicate env vars to file Supply WP related env vars to CLI container as well Debug wp config Remove --quiet flag Use alternative method to execute SQL command Create db before ensuring connection to WordPress Debug mysql command Disable pseudo-tty allocation for docker-compose command Use latest WP image for local env; create database during env setup Update workflow name Use alternative workflow ID Rename workflow Don't run workflow if certain files change Cancel any previous queued or in-progress workflow runs on push to a branch Reduce specificity of CSS selector ...

westonruter · 2021-03-16T04:59:29Z

Sorry for the delay in reviewing.

@delawski For validation errors which are reviewed, could you add a white left-border or add additional padding so that the unreviewed and reviewed have consistent alignment:

westonruter · 2021-03-16T05:02:07Z

@delawski Also, where did the link go to access the Validated URL screen? There should be a details link still.

westonruter · 2021-03-16T05:12:49Z

The error titles can be much longer than just “invalid element”, especially when translated. Currently the client-side logic is doing something hacky by truncating the title at the first colon. So instead of Invalid element: <code>bad</code> it becomes just Invalid element. This doesn't always work, for example:

I think we need to:

Discontinue the truncation at the colon.
Not have “kept” label spelled out. Having just the icon should be fine because they can expand to understand what the icon means.

Here's a more extreme example of how long the titles can be:

Naturally the <code> markup should not be getting escaped in this scenario.

westonruter · 2021-03-19T02:03:10Z

assets/src/block-validation/amp-validation-status/status-notification.js

+	if ( isEditedPostNew ) {
+		return (
+			<SidebarNotification
+				icon={ <StatusIcon /> }
+				message={ __( 'Validation will be checked upon saving.', 'amp' ) }
+			/>
+		);
+	}


Would it make sense to move this condition up to be the first after the isFetchingErrors conditional?

That makes sense, yes.

Done in 56ceb8c.

pierlon · 2021-03-19T02:39:38Z

Would it make sense adding titles for each section? At the moment they seem bare and the user may not immediately know what they're looking at.

pierlon · 2021-03-19T03:08:14Z

While testing this I found given a HTML block with the content <?php, a validation error is only raised when it is the first block on the page, which is weird. Here's an example:

potential_bug.mp4

pierlon · 2021-03-19T03:11:54Z

Bug replicated on the develop branch as well, so it's not from this PR. Will investigate.

assets/src/block-validation/sidebar.js

pierlon · 2021-03-19T03:29:42Z

assets/src/block-validation/sidebar.js

-						</PanelBody>
-					)
+			{ hasReviewedValidationErrors && (
+				<div className="amp-sidebar__options">


Would it be better to place this above the list of errors? One would potentially have to scroll all the way down to see this button (due to viewport constraints or having many errors).

The idea is that this would be clicked infrequently. The main thing a user will care about are the unreviewed issues. The link to show all is there just for curiosity.

westonruter · 2021-03-19T04:18:03Z

Would it make sense adding titles for each section? At the moment they seem bare and the user may not immediately know what they're looking at.

Maybe the entire sidebar should be called "AMP Validation" instead of just "AMP" for now?

pierlon · 2021-03-19T04:28:07Z

Maybe the entire sidebar should be called "AMP Validation" instead of just "AMP" for now?

That could work as well.

westonruter · 2021-03-19T06:00:16Z

While testing this I found given a HTML block with the content <?php, a validation error is only raised when it is the first block on the page, which is weird. Here's an example:

@pierlon It's simply because your <script> tag was not closed properly. If I look at the non-AMP version the syntax highlighting indicates as such:

Also see this example code:

<?php

use AmpProject\Dom\Document;

$script_first = '<div>
	<script</script>
	<?php
	?>
</div>';

$pi_first = '<div>
	<?php
	?>
	<script</script>
</div>';

$script_first_doc = Document::fromHtmlFragment( $script_first );
$pi_first_doc = Document::fromHtmlFragment( $pi_first );

echo "## script_first_doc\n";
echo $script_first_doc->saveHTML( $script_first_doc->body );

echo "\n\n";

echo "## pi_first_doc\n";
echo $pi_first_doc->saveHTML( $pi_first_doc->body );

Output:

## script_first_doc
<body><div>
	<script>
	<?php
	?>
</script></div></body>

## pi_first_doc
<body><div>
	<?php ?>
	<script>
</script></div></body>

Notice the PHP processing instruction gets rendered inside the script in the first example, and the PHP PI is there considered text content not markup. So it is not a separate validation error.

westonruter · 2021-03-19T06:04:29Z

Maybe the entire sidebar should be called "AMP Validation" instead of just "AMP" for now?

I've made this change:

In the future when/if we add non-validation stuff to the sidebar, we can change it back to "AMP".

westonruter · 2021-03-19T06:25:28Z

@delawski Would you open a new issue for adding the pre-publish panel?

delawski · 2021-03-19T14:53:26Z

@pierlon @westonruter I've addressed your feedback. I think the only thing left to do is to update the unit tests for the StatusNotification which I'll do shortly.

Would you open a new issue for adding the pre-publish panel?

Yes, I'll create a single ticket for the pre-publish panel and for the panel in the document sidebar.

delawski · 2021-03-19T15:10:20Z

assets/src/block-validation/amp-validation-status/revalidate-notification.js

+			const blockDetails = clientId ? select( 'core/block-editor' ).getBlock( clientId ) : null;
+			if ( clientId && ! blockDetails ) {


As in aa668d0, we could get here just the block name instead of the entire block object, i.e.

Suggested change

const blockDetails = clientId ? select( 'core/block-editor' ).getBlock( clientId ) : null;

if ( clientId && ! blockDetails ) {

if ( clientId && ! select( 'core/block-editor' ).getBlockName( clientId ) ) {

When I think about it, we could actually replace the entire loop with find method like

- let _hasErrorsFromRemovedBlocks = false; - for ( const validationError of select( BLOCK_VALIDATION_STORE_KEY ).getValidationErrors() ) { - const { clientId } = validationError; - const blockDetails = clientId ? select( 'core/block-editor' ).getBlock( clientId ) : null; - if ( clientId && ! blockDetails ) { - _hasErrorsFromRemovedBlocks = true; - break; - } - } - - return { - hasErrorsFromRemovedBlocks: _hasErrorsFromRemovedBlocks, + const errors = select( BLOCK_VALIDATION_STORE_KEY ).getValidationErrors(); + + return { + hasErrorsFromRemovedBlocks: Boolean( errors.find( ( { clientId } ) => clientId && ! select( 'core/block-editor' ).getBlockName( clientId ) ) ),

But I guess it might not be as readable as the loop.

I like that latter suggestion. Go ahead with it!

Sounds good, it's done (in an even shorter form) in 17ef326.

delawski · 2021-03-19T15:33:02Z

The unit tests should now be good (see: 4794812).

Would you open a new issue for adding the pre-publish panel?

Yes, I'll create a single ticket for the pre-publish panel and for the panel in the document sidebar.

Here's the follow-up ticket: #5997

westonruter

A thing of beauty!

It is a gift!

pierlon · 2021-03-19T16:20:27Z

@pierlon It's simply because your <script> tag was not closed properly...

Ah that makes sense. Thanks for solving the mystery @westonruter 🔍.

pierlon

Marvelous work done here 🏆.

assets/src/block-editor/components/sidebar-notification/index.js

pierlon · 2021-03-19T16:48:27Z

assets/src/block-validation/amp-validation-status/revalidate-notification.js

+		hasErrorsFromRemovedBlocks: Boolean( select( BLOCK_VALIDATION_STORE_KEY ).getValidationErrors().find(
+			( { clientId } ) => clientId && ! select( 'core/block-editor' ).getBlockName( clientId ) ),
+		),


Nicely done 👍

delawski added 7 commits February 25, 2021 17:29

Introduce editor sidebar notification component

e7b63c8

Introduce AMP status icon to be used in sidebar notifications

6ea0cb6

Set isFetchingErrors flag when editor loads

bba583a

Extract validation status messages to notification components

6baa417

Update the reviewed errors toggle appearance

dd50f6c

Use uniform CSS naming convention for Icon and add unit tests

fdd63e2

Do not show revalidate notification when fetching validation errors

99344da

delawski added this to the v2.1 milestone Feb 25, 2021

delawski requested review from westonruter and pierlon February 25, 2021 17:56

delawski self-assigned this Feb 25, 2021

Add unit test to cover validation errors fetching state

d729b5a

delawski mentioned this pull request Feb 26, 2021

Replace synchronous URL validation in editor with async validation #5741

Merged

3 tasks

Base automatically changed from feature/2069-async-url-validation to develop March 2, 2021 03:25

delawski added 4 commits March 8, 2021 11:59

Merge branch 'develop' into feature/5304-validation-notifications

6916764

# Conflicts: # assets/src/block-validation/sidebar.js

Add loading state message and reorder notifications in sidebar

543ab48

Add API request error handling

c37125e

Remove reference to 'post' in pending-save message

6f678f2

westonruter reviewed Mar 19, 2021

View reviewed changes

pierlon reviewed Mar 19, 2021

View reviewed changes

Change block editor plugin title from 'AMP' to 'AMP Validation'

d1c05da

delawski added 4 commits March 19, 2021 15:13

Remove unnecessary optional chaining operator

cb830a0

Use more exact types in JSDoc and propTypes definition

efeba44

Get blockName directly without other properties

aa668d0

Check if post is new early

56ceb8c

delawski commented Mar 19, 2021

View reviewed changes

Fix unit tests and update snapshots

4794812

delawski requested review from westonruter and pierlon March 19, 2021 15:25

delawski mentioned this pull request Mar 19, 2021

Add AMP status notification to the pre-publish panel and to the AMP panel in the document sidebar #5997

Closed

Use array find method instead of loop

17ef326

westonruter approved these changes Mar 19, 2021

View reviewed changes

pierlon approved these changes Mar 19, 2021

View reviewed changes

westonruter merged commit bcd47bf into develop Mar 19, 2021

westonruter deleted the feature/5304-validation-notifications branch March 19, 2021 17:01

delawski mentioned this pull request Mar 30, 2021

Add status notification to the pre-publish and document sidebar panels #6027

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update editor sidebar validation notifications #5929

Update editor sidebar validation notifications #5929

delawski commented Feb 25, 2021 •

edited

Loading

codecov bot commented Feb 25, 2021 •

edited

Loading

github-actions bot commented Feb 25, 2021 •

edited

Loading

westonruter commented Mar 2, 2021

delawski commented Mar 2, 2021

jwold commented Mar 2, 2021

delawski commented Mar 4, 2021

delawski commented Mar 8, 2021

jwold commented Mar 9, 2021

westonruter commented Mar 16, 2021

westonruter commented Mar 16, 2021

westonruter commented Mar 16, 2021

westonruter Mar 19, 2021

pierlon Mar 19, 2021

delawski Mar 19, 2021

pierlon commented Mar 19, 2021

pierlon commented Mar 19, 2021

pierlon commented Mar 19, 2021

pierlon Mar 19, 2021

westonruter Mar 19, 2021

westonruter commented Mar 19, 2021

pierlon commented Mar 19, 2021

westonruter commented Mar 19, 2021

westonruter commented Mar 19, 2021

westonruter commented Mar 19, 2021

delawski commented Mar 19, 2021

delawski Mar 19, 2021

delawski Mar 19, 2021

westonruter Mar 19, 2021

delawski Mar 19, 2021

delawski commented Mar 19, 2021

westonruter left a comment

pierlon commented Mar 19, 2021 •

edited

Loading

pierlon left a comment

pierlon Mar 19, 2021

		const blockDetails = clientId ? select( 'core/block-editor' ).getBlock( clientId ) : null;
		if ( clientId && ! blockDetails ) {

	const blockDetails = clientId ? select( 'core/block-editor' ).getBlock( clientId ) : null;
	if ( clientId && ! blockDetails ) {
	if ( clientId && ! select( 'core/block-editor' ).getBlockName( clientId ) ) {

Update editor sidebar validation notifications #5929

Update editor sidebar validation notifications #5929

Conversation

delawski commented Feb 25, 2021 • edited Loading

Summary

Checklist

codecov bot commented Feb 25, 2021 • edited Loading

Codecov Report

github-actions bot commented Feb 25, 2021 • edited Loading

westonruter commented Mar 2, 2021

delawski commented Mar 2, 2021

jwold commented Mar 2, 2021

delawski commented Mar 4, 2021

delawski commented Mar 8, 2021

jwold commented Mar 9, 2021

westonruter commented Mar 16, 2021

westonruter commented Mar 16, 2021

westonruter commented Mar 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierlon commented Mar 19, 2021

pierlon commented Mar 19, 2021

pierlon commented Mar 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented Mar 19, 2021

pierlon commented Mar 19, 2021

westonruter commented Mar 19, 2021

westonruter commented Mar 19, 2021

westonruter commented Mar 19, 2021

delawski commented Mar 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delawski commented Mar 19, 2021

westonruter left a comment

Choose a reason for hiding this comment

pierlon commented Mar 19, 2021 • edited Loading

pierlon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delawski commented Feb 25, 2021 •

edited

Loading

codecov bot commented Feb 25, 2021 •

edited

Loading

github-actions bot commented Feb 25, 2021 •

edited

Loading

pierlon commented Mar 19, 2021 •

edited

Loading