Delay showing the unreviewed counts of validated URLs and errors; remove 'At a Glance' widget #5900

pierlon · 2021-02-19T03:19:49Z

Summary

Fixes #5772

Removes 'At a Glance' widget on dashboard that diplays summary of unreviewed count of validated URLs
Delays showing the unreviewed counts of validated URLs and validation errors by asynchronously updating the 'Validated URLs' and 'Error Index' menu items once they are visible

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

…ate respective menu items with counts

codecov · 2021-02-19T05:41:11Z

Codecov Report

Merging #5900 (99165f8) into develop (26034a7) will decrease coverage by 0.19%.
The diff coverage is 57.14%.

@@              Coverage Diff              @@
##             develop    #5900      +/-   ##
=============================================
- Coverage      75.16%   74.96%   -0.20%     
- Complexity      5665     5675      +10     
=============================================
  Files            210      212       +2     
  Lines          17019    17049      +30     
=============================================
- Hits           12792    12781      -11     
- Misses          4227     4268      +41

Flag	Coverage Δ	Complexity Δ
javascript	`75.13% <ø> (ø)`	`0.00 <ø> (ø)`
php	`74.96% <57.14%> (-0.21%)`	`0.00 <15.00> (ø)`

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

Impacted Files	Coverage Δ	Complexity Δ
includes/utils/class-amp-dom-utils.php	`91.56% <ø> (ø)`	`35.00 <0.00> (ø)`
src/AmpWpPlugin.php	`100.00% <ø> (ø)`	`8.00 <0.00> (ø)`
src/DevTools/UserAccess.php	`100.00% <ø> (ø)`	`22.00 <0.00> (ø)`
src/Admin/ValidationCounts.php	`35.00% <35.00%> (ø)`	`4.00 <4.00> (?)`
...s/validation/class-amp-validated-url-post-type.php	`64.74% <50.00%> (-1.35%)`	`442.00 <2.00> (-5.00)`
src/Validation/ValidationCountsRestController.php	`62.50% <62.50%> (ø)`	`7.00 <7.00> (?)`
includes/sanitizers/class-amp-meta-sanitizer.php	`79.79% <75.00%> (-0.21%)`	`35.00 <0.00> (+3.00)`	⬇️
...validation/class-amp-validation-error-taxonomy.php	`50.26% <100.00%> (-0.04%)`	`540.00 <0.00> (-1.00)`
src/Validation/SavePostValidationEvent.php	`91.66% <100.00%> (-2.46%)`	`18.00 <1.00> (+1.00)`	⬇️
src/Validation/URLValidationCron.php	`73.68% <100.00%> (-20.44%)`	`8.00 <1.00> (+1.00)`	⬇️
... and 6 more

pierlon · 2021-02-19T20:20:37Z

This is ready for review while I work on resolving these E2E test issues.

assets/src/amp-validation/counts/index.js

src/Admin/ValidationCounts.php

Co-authored-by: Weston Ruter <[email protected]>

westonruter · 2021-02-20T04:51:37Z

assets/src/amp-validation/counts/index.js

+	const counts = await apiFetch( { path: '/amp/v1/unreviewed-validation-counts' } );
+	updateMenuItemCounts( counts );


I'm looking at apiFetch here:

https://github.com/WordPress/gutenberg/blob/30b2c144afc8586602c419d2faf21b7f9721be7b/packages/api-fetch/src/index.js#L57-L63

It's throwing an error if the request fails including if a non-successful response code is being sent. Therefore, I think actually updateMenuItemCounts can be changed to only accept an object and that this code here can be changed to:

Suggested change

const counts = await apiFetch( { path: '/amp/v1/unreviewed-validation-counts' } );

updateMenuItemCounts( counts );

try {

const counts = await apiFetch( { path: '/amp/v1/unreviewed-validation-counts' } );

if ( ! isPlainObject( counts ) ) {

throw new Error( 'Object not returned.' );

}

updateMenuItemCounts( counts );

} catch ( exception ) {

// eslint-disable-next-line no-console

console.error( '[AMP Plugin] An error occurred while retrieving unreviewed validation counts.', exception.message );

}

Ah, right. I've taken a similar approach to what you've suggested in 70941a5.

assets/src/amp-validation/counts/index.js

westonruter · 2021-02-20T05:30:22Z

assets/src/amp-validation/counts/index.js

+	const errorCountEl = document.getElementById( 'new-error-index-count' );
+	const validatedUrlsCountEl = document.getElementById( 'new-validation-url-count' );
+
+	updateMenuItem( errorCountEl, newErrorCount );
+	updateMenuItem( validatedUrlsCountEl, newValidatedUrlCount );


This can be hardened a bit to handle other plugins which may have removed the elements from the admin menu:

Suggested change

const errorCountEl = document.getElementById( 'new-error-index-count' );

const validatedUrlsCountEl = document.getElementById( 'new-validation-url-count' );

updateMenuItem( errorCountEl, newErrorCount );

updateMenuItem( validatedUrlsCountEl, newValidatedUrlCount );

const errorCountEl = document.getElementById( 'new-error-index-count' );

if ( errorCountEl ) {

updateMenuItem( errorCountEl, newErrorCount );

}

const validatedUrlsCountEl = document.getElementById( 'new-validation-url-count' );

if ( validatedUrlsCountEl ) {

updateMenuItem( validatedUrlsCountEl, newValidatedUrlCount );

}

Updated in 8d90f2d.

westonruter · 2021-02-20T05:31:53Z

assets/src/amp-validation/counts/index.js

+	count = Math.abs( count );
+


I'm thinking abs() may be unnecessary here?

Suggested change

count = Math.abs( count );

In retrospect, yea not necessary. Removed in 8d90f2d.

westonruter · 2021-02-20T05:34:22Z

This is a very nice loading indicator:

spinners.mov

westonruter · 2021-02-20T05:40:02Z

assets/src/amp-validation/counts/index.js

+}
+
+domReady( async () => {
+	const counts = await apiFetch( { path: '/amp/v1/unreviewed-validation-counts' } );


Another performance enhancement would be to check whether the #toplevel_page_amp-options element has the wp-has-current-submenu class meaning the submenu is expanded. If it does, then go ahead and fetch and display the counts. Otherwise, if it does not have this class (or if it has the wp-not-current-submenu class), then the menu is not expanded and it is not needed to do the fetch immediately. It can be deferred until the first time that the opensub class is added to this element, which is what happens when the user hovers over the collapsed menu item or tabs onto it. In this way, the REST API request will only be triggered with every admin page load if the user is looking at one of the AMP screens. If not, then the request will be deferred until the point that the user is actually going to see the menu.

Actually, a nice way to implement both could be to wrap this logic in an IntersectionObserver callback, specifically targeting the parent element of the menu items. This should defer the loading of the data for 3 scenarios:

The user is not on an AMP screen so the menu items are not shown until the AMP menu is hovered/focus.

The browser window is too short to even show the initially-expanded AMP settings menu in the first viewport, meaning the fetch could be delayed until the user scrolls down.

The user has collapsed the entire admin menu in which case all of the submenus are hidden, even those which have the wp-has-current-submenu class.

That worked perfectly (see 70941a5), thanks for the tip! I've set it to fetch the counts whenever the AMP submenu is visible.

pierlon · 2021-02-22T21:35:27Z

We may have another flaky E2E test here due to the async loading of counts; confirming if that's the case...

github-actions · 2021-02-22T21:45:49Z

Plugin builds for f1005a9 are ready 🛎️!

Download development build
Download production build

pierlon · 2021-02-22T22:13:24Z

We may have another flaky E2E test here due to the async loading of counts; confirming if that's the case...

This doesn't seem to be the case.

…hiding counts

westonruter

This is slick.

westonruter · 2021-02-23T18:27:18Z

Tested that behavior is consistent across Chrome, Firefox, and Safari.

pierlon added 4 commits February 18, 2021 21:59

Remove 'At a Glance' filter for showing validation URL count summary

8ba7980

Fetch unreviewed counts of validated URLs & errors via AJAX call; upd…

8b7baf5

…ate respective menu items with counts

Add tests

e7bbd91

Fix lint issue

df9c825

pierlon requested a review from westonruter February 19, 2021 20:20

pierlon commented Feb 19, 2021

View reviewed changes

assets/src/amp-validation/counts/index.js Outdated Show resolved Hide resolved

westonruter reviewed Feb 19, 2021

View reviewed changes

assets/src/amp-validation/counts/index.js Show resolved Hide resolved

westonruter reviewed Feb 19, 2021

View reviewed changes

src/Admin/ValidationCounts.php Outdated Show resolved Hide resolved

westonruter reviewed Feb 19, 2021

View reviewed changes

src/Admin/ValidationCounts.php Outdated Show resolved Hide resolved

westonruter reviewed Feb 19, 2021

View reviewed changes

src/Admin/ValidationCounts.php Outdated Show resolved Hide resolved

pierlon and others added 3 commits February 20, 2021 02:17

Register service on admin_enqueue_scripts action

5c83722

Co-authored-by: Weston Ruter <[email protected]>

Load script in footer

911bbd0

Co-authored-by: Weston Ruter <[email protected]>

Update jsdoc for counts parameter

14fd4b2

pierlon force-pushed the fix/5772-delayed-validation-error-count-display branch from 41a546e to 14fd4b2 Compare February 20, 2021 02:23

Update ValidationCounts test

9c43ee6

westonruter reviewed Feb 20, 2021

View reviewed changes

assets/src/amp-validation/counts/index.js Outdated Show resolved Hide resolved

westonruter reviewed Feb 20, 2021

View reviewed changes

pierlon added 3 commits February 22, 2021 15:32

Use intersection observer to determine when to fetch validation counts

70941a5

Harden updates of submenu items

8d90f2d

Add browser env global support to eslint

2a09ef1

pierlon requested a review from westonruter February 22, 2021 21:19

pierlon self-assigned this Feb 22, 2021

pierlon added this to the v2.1 milestone Feb 22, 2021

pierlon added the WS:Core Work stream for Plugin core label Feb 22, 2021

pierlon marked this pull request as ready for review February 22, 2021 21:19

Remove duplicate declaration of global vars

de08cbd

pierlon force-pushed the fix/5772-delayed-validation-error-count-display branch from 2b8d478 to de08cbd Compare February 22, 2021 21:40

pierlon added WS:Perf Work stream for Metrics, Performance and Optimizer and removed WS:Core Work stream for Plugin core labels Feb 23, 2021

westonruter added 2 commits February 22, 2021 21:00

Account for IntersectionObserver not being supported in IE11 by just …

ed31fe3

…hiding counts

Reuse target to unobserve

f1005a9

westonruter approved these changes Feb 23, 2021

View reviewed changes

westonruter merged commit f2af134 into develop Feb 23, 2021

westonruter deleted the fix/5772-delayed-validation-error-count-display branch February 23, 2021 18:38

westonruter mentioned this pull request Dec 6, 2021

Red loader at AMP menu, without validation errors #6765

Closed

delawski mentioned this pull request Dec 14, 2021

Preload validation counts data if AMP submenu is expanded #6770

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delay showing the unreviewed counts of validated URLs and errors; remove 'At a Glance' widget #5900

Delay showing the unreviewed counts of validated URLs and errors; remove 'At a Glance' widget #5900

pierlon commented Feb 19, 2021 •

edited

Loading

codecov bot commented Feb 19, 2021 •

edited

Loading

pierlon commented Feb 19, 2021

westonruter Feb 20, 2021

pierlon Feb 22, 2021 •

edited

Loading

westonruter Feb 20, 2021

pierlon Feb 22, 2021

westonruter Feb 20, 2021

pierlon Feb 22, 2021

westonruter commented Feb 20, 2021

westonruter Feb 20, 2021

westonruter Feb 20, 2021

pierlon Feb 22, 2021 •

edited

Loading

pierlon commented Feb 22, 2021

github-actions bot commented Feb 22, 2021 •

edited

Loading

pierlon commented Feb 22, 2021

westonruter left a comment

westonruter commented Feb 23, 2021

		const counts = await apiFetch( { path: '/amp/v1/unreviewed-validation-counts' } );
		updateMenuItemCounts( counts );

-	const counts = await apiFetch( { path: '/amp/v1/unreviewed-validation-counts' } );
-	updateMenuItemCounts( counts );
+	try {
+		const counts = await apiFetch( { path: '/amp/v1/unreviewed-validation-counts' } );
+		if ( ! isPlainObject( counts ) ) {
+			throw new Error( 'Object not returned.' );
+		}
+		updateMenuItemCounts( counts );
+	} catch ( exception ) {
+		// eslint-disable-next-line no-console
+		console.error( '[AMP Plugin] An error occurred while retrieving unreviewed validation counts.', exception.message );
+	}

Delay showing the unreviewed counts of validated URLs and errors; remove 'At a Glance' widget #5900

Delay showing the unreviewed counts of validated URLs and errors; remove 'At a Glance' widget #5900

Conversation

pierlon commented Feb 19, 2021 • edited Loading

Summary

Checklist

codecov bot commented Feb 19, 2021 • edited Loading

Codecov Report

pierlon commented Feb 19, 2021

westonruter Feb 20, 2021

Choose a reason for hiding this comment

pierlon Feb 22, 2021 • edited Loading

Choose a reason for hiding this comment

westonruter Feb 20, 2021

Choose a reason for hiding this comment

pierlon Feb 22, 2021

Choose a reason for hiding this comment

westonruter Feb 20, 2021

Choose a reason for hiding this comment

pierlon Feb 22, 2021

Choose a reason for hiding this comment

westonruter commented Feb 20, 2021

westonruter Feb 20, 2021

Choose a reason for hiding this comment

westonruter Feb 20, 2021

Choose a reason for hiding this comment

pierlon Feb 22, 2021 • edited Loading

Choose a reason for hiding this comment

pierlon commented Feb 22, 2021

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

pierlon commented Feb 22, 2021

westonruter left a comment

Choose a reason for hiding this comment

westonruter commented Feb 23, 2021

pierlon commented Feb 19, 2021 •

edited

Loading

codecov bot commented Feb 19, 2021 •

edited

Loading

pierlon Feb 22, 2021 •

edited

Loading

pierlon Feb 22, 2021 •

edited

Loading

github-actions bot commented Feb 22, 2021 •

edited

Loading