Preload validation counts data if AMP submenu is expanded #6770

delawski · 2021-12-07T19:03:43Z

Summary

So far, the Validated URLs and Error Index counts (validation counts in short) in WP admin were fetched from the REST API whenever the AMP submenu has been expanded. This approach has been introduced in #5900 in order to limit server resources usage - it's useless to calculate the validation counts if the AMP submenu is never revealed.

As a side-effect, a UX issue as described in #6765 has been introduced. If the AMP submenu is initially expanded, e.g. a user is on the AMP Settings screen or is browsing Validated URLs, the validation counts are still fetched asynchronously from REST API causing a momentary flash of a loading spinner inside the red badge.

In this PR, we take advantage of apiFetch preload middleware for providing the validation counts on page load but only if the AMP submenu is initially expanded. For other cases, we keep use the same MutationObserver-based solution so that the validation counts are still not calculated needlessly.

Before

validation-counts-before.mp4

After

validation-counts-after.mp4

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

delawski · 2021-12-14T13:16:40Z

src/Admin/ValidationCounts.php

+	 */
+	protected function maybe_add_preload_rest_paths() {
+		if (
+			AMP_Options_Manager::OPTION_NAME === get_admin_page_parent()


I couldn't find a way to cover this condition with a unit test. I'm not sure how can I override get_admin_page_parent() in the test suite.

I've found a (hacky) way to do so in 7981cb0.

codecov · 2021-12-14T13:17:04Z

Codecov Report

Merging #6770 (f3ecdd1) into develop (e0e4014) will increase coverage by 0.01%.
The diff coverage is 81.81%.

❗ Current head f3ecdd1 differs from pull request most recent head 090fb5d. Consider uploading reports for the commit 090fb5d to get more accurate results

@@              Coverage Diff              @@
##             develop    #6770      +/-   ##
=============================================
+ Coverage      77.47%   77.48%   +0.01%     
- Complexity      6704     6708       +4     
=============================================
  Files            267      267              
  Lines          21378    21388      +10     
=============================================
+ Hits           16563    16573      +10     
  Misses          4815     4815

Flag	Coverage Δ
javascript	`63.42% <ø> (-0.05%)`	⬇️
php	`78.30% <81.81%> (+0.01%)`	⬆️
unit	`78.30% <81.81%> (+0.01%)`	⬆️

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

Impacted Files	Coverage Δ
src/Admin/ValidationCounts.php	`51.51% <77.77%> (+14.01%)`	⬆️
...s/validation/class-amp-validated-url-post-type.php	`67.32% <100.00%> (ø)`
...validation/class-amp-validation-error-taxonomy.php	`53.65% <100.00%> (ø)`
...g-wizard/components/navigation-context-provider.js	`79.16% <0.00%> (-5.84%)`	⬇️
src/Admin/OnboardingWizardSubmenuPage.php	`91.12% <0.00%> (-0.15%)`	⬇️
includes/amp-helper-functions.php	`84.88% <0.00%> (-0.03%)`	⬇️
src/Admin/GoogleFonts.php	`50.00% <0.00%> (+21.42%)`	⬆️

github-actions · 2021-12-14T13:20:12Z

Plugin builds for 7981cb0 are ready 🛎️!

Download development build
Download production build

…min-validation-counts * 'develop' of github.com:ampproject/amp-wp: Skip Technical Background step if there is no dependency support Allow excessive CSS in Customizer preview

westonruter · 2021-12-14T18:18:32Z

@delawski I have amended this with another small enhancement. Namely, in the case where there are no validation counts, previously there would be a flash of the loading spinner in both when the menu was pre-expanded and when it was expanded on hover:

flash-of-loading-counts.mov

In bc07073 I've made use of sessionStorage to remember the empty counts case to prevent that flash of the loading spinner:

flash-of-loading-counts.mov

If it happens that new validation counts have been generated after previously none were returned, then the result is just that no spinner appears before the counts are added:

no-loading-spinner-when-previously-zero-counts.mov

I think this may offer a marginally better experience. Let me know what you think.

delawski · 2021-12-15T10:27:10Z

Thanks for the amendment. I think the UX is even better now. Everything works as expected and the code looks good.

dhaval-parekh

Changes looks good me.

fellyph

I wasn't getting the spinner when I didn't have errors:

AMP.Settings.tweety-two.WordPress.mp4

But also wasn't updating when I have new errors:

AMP.Settings.tweety-two.WordPress.1.mp4

delawski · 2021-12-15T14:42:04Z

@fellyph I wasn't able to reproduce this issue:

validation-counts.mp4

Can you provide your test steps?

maitreyie-chavan · 2021-12-15T14:45:28Z

Agreeing with @delawski, I was unable to reproduce this at my end as well.

fellyph · 2021-12-15T15:32:12Z

@delawski I was using WordPress 5.9 beta 3, but now testing develop branch, I'm not getting the issue anymore on WordPress 5.8 and 5.9 beta 3.

This comment has been minimized.

Sign in to view

westonruter added this to the v2.3 milestone Dec 7, 2021

delawski mentioned this pull request Dec 7, 2021

Red loader at AMP menu, without validation errors #6765

Closed

This comment has been minimized.

Sign in to view

delawski added 3 commits December 14, 2021 13:41

Preload validation counts data if AMP submenu is expanded

b846627

Add unit tests

afcea3c

Clean up code and improve methods names

5694ce5

delawski force-pushed the update/admin-validation-counts branch from 931b666 to 5694ce5 Compare December 14, 2021 12:41

Remove superfluous argument and move up common function call

f3ecdd1

delawski marked this pull request as ready for review December 14, 2021 13:14

delawski commented Dec 14, 2021

View reviewed changes

delawski requested review from westonruter and dhaval-parekh December 14, 2021 13:17

delawski added Bug Something isn't working Validation labels Dec 14, 2021

westonruter added 3 commits December 14, 2021 09:39

Merge branch 'develop' of github.com:ampproject/amp-wp into update/ad…

19ffbfd

…min-validation-counts * 'develop' of github.com:ampproject/amp-wp: Skip Technical Background step if there is no dependency support Allow excessive CSS in Customizer preview

Add amp prefix to element IDs

090fb5d

Use sessionStorage to avoid flash of loading zero counts

bc07073

westonruter modified the milestones: v2.3, v2.2 Dec 14, 2021

westonruter requested a review from fellyph December 14, 2021 18:19

westonruter added the P1 Medium priority label Dec 14, 2021

Hackily set environment to test get_admin_page_parent()

7981cb0

westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Dec 15, 2021

dhaval-parekh approved these changes Dec 15, 2021

View reviewed changes

fellyph reviewed Dec 15, 2021

View reviewed changes

westonruter merged commit 2fca9b9 into develop Dec 15, 2021

westonruter deleted the update/admin-validation-counts branch December 15, 2021 14:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preload validation counts data if AMP submenu is expanded #6770

Preload validation counts data if AMP submenu is expanded #6770

delawski commented Dec 7, 2021 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

delawski Dec 14, 2021

westonruter Dec 14, 2021

codecov bot commented Dec 14, 2021 •

edited

Loading

github-actions bot commented Dec 14, 2021 •

edited

Loading

westonruter commented Dec 14, 2021

delawski commented Dec 15, 2021

dhaval-parekh left a comment

fellyph left a comment

delawski commented Dec 15, 2021

maitreyie-chavan commented Dec 15, 2021 •

edited

Loading

fellyph commented Dec 15, 2021 •

edited

Loading

Preload validation counts data if AMP submenu is expanded #6770

Preload validation counts data if AMP submenu is expanded #6770

Conversation

delawski commented Dec 7, 2021 • edited Loading

Summary

Checklist

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

delawski Dec 14, 2021

Choose a reason for hiding this comment

westonruter Dec 14, 2021

Choose a reason for hiding this comment

codecov bot commented Dec 14, 2021 • edited Loading

Codecov Report

github-actions bot commented Dec 14, 2021 • edited Loading

westonruter commented Dec 14, 2021

delawski commented Dec 15, 2021

dhaval-parekh left a comment

Choose a reason for hiding this comment

fellyph left a comment

Choose a reason for hiding this comment

delawski commented Dec 15, 2021

maitreyie-chavan commented Dec 15, 2021 • edited Loading

fellyph commented Dec 15, 2021 • edited Loading

delawski commented Dec 7, 2021 •

edited

Loading

codecov bot commented Dec 14, 2021 •

edited

Loading

github-actions bot commented Dec 14, 2021 •

edited

Loading

maitreyie-chavan commented Dec 15, 2021 •

edited

Loading

fellyph commented Dec 15, 2021 •

edited

Loading