Skip to content

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

Merged
westonruter merged 25 commits intodevelopfrom
update/5997-editor-sidebar-notifications
Apr 13, 2021
Merged

Add status notification to the pre-publish and document sidebar panels#6027
westonruter merged 25 commits intodevelopfrom
update/5997-editor-sidebar-notifications

Conversation

@delawski
Copy link
Copy Markdown
Collaborator

@delawski delawski commented Mar 30, 2021

Summary

Fixes #5997

This PR introduces AMP document status notifications to the pre-publish panel and to the general document sidebar.

Pre-publish Panel Document Sidebar
Screenshot 2021-03-30 at 16 57 12 Screenshot 2021-03-30 at 16 57 28

It also contains some updates to the existing notifications area introduced in #5929.

Note that the base for this PR is #6022 that reorgs and cleans up the block-validation directory.

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 delawski added the WS:UX Work stream for UX/Front-end label Mar 30, 2021
@delawski delawski requested review from pierlon and westonruter March 30, 2021 15:05
@delawski delawski self-assigned this Mar 30, 2021
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 30, 2021 •
edited
Loading

Codecov Report

Merging #6027 (f56c038) into develop (2d0de18) will increase coverage by 0.12%.
The diff coverage is 92.85%.

❗ Current head f56c038 differs from pull request most recent head 5a7860a. Consider uploading reports for the commit 5a7860a to get more accurate results
Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #6027      +/-   ##
=============================================
+ Coverage      75.27%   75.39%   +0.12%     
+ Complexity      5709     5706       -3     
=============================================
  Files            218      225       +7     
  Lines          17283    17323      +40     
=============================================
+ Hits           13009    13061      +52     
+ Misses          4274     4262      -12
Flag Coverage Δ Complexity Δ
javascript 79.84% <94.82%> (-0.22%) 0.00 <0.00> (ø)
php 75.19% <88.46%> (+0.12%) 0.00 <4.00> (ø)

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

Impacted Files Coverage Δ Complexity Δ
amp.php 0.00% <ø> (ø) 0.00 <0.00> (ø)
...lidation/components/amp-validation-status/index.js 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...alidation/components/sidebar-notification/index.js 100.00% <ø> (ø) 0.00 <0.00> (ø)
...s/src/block-validation/components/sidebar/index.js 0.00% <ø> (ø) 0.00 <0.00> (?)
includes/amp-helper-functions.php 84.48% <ø> (-0.06%) 0.00 <0.00> (ø)
includes/embeds/class-amp-core-block-handler.php 87.38% <ø> (-3.60%) 43.00 <0.00> (-8.00)
...udes/sanitizers/class-amp-core-theme-sanitizer.php 36.28% <ø> (+0.41%) 228.00 <0.00> (-1.00) ⬆️
src/Transformer/DetermineHeroImages.php 91.04% <ø> (ø) 25.00 <0.00> (ø)
includes/class-amp-theme-support.php 86.22% <70.00%> (-0.33%) 245.00 <0.00> (+3.00) ⬇️
...k-validation/hooks/use-post-dirty-state-changes.js 96.96% <75.00%> (-3.04%) 0.00 <0.00> (ø)
... and 21 more

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 30, 2021 •
edited
Loading

Plugin builds for 5a7860a are ready 🛎️!

Base automatically changed from add/5997-editor-sidebar-notifications to develop March 30, 2021 23:04
@delawski
Copy link
Copy Markdown
Collaborator Author

delawski commented Apr 6, 2021

This time the failing E2E test is most likely an actual issue caused by missing (moved) "Enable AMP" toggle. I'll see if I can fix this test tomorrow.

delawski added 3 commits April 10, 2021 18:18
@delawski
Fix broken E2E test
954682f
@delawski
Add more unit tests
b243318
@delawski
Ignore block editor plugins in unit test
019a100 
Since Gutenberg plugins do not render markup directly, covering them with unit tests is not practical. Instead - if really needed - they should be covered by E2E tests.

By ignoring the plugins we ensure that the code coverage report is not negatively impacted.
delawski
delawski commented Apr 10, 2021
View reviewed changes
tests/js/jest.config.js
Comment on lines +16 to +17
'<rootDir>/assets/src/block-editor/plugins',
'<rootDir>/assets/src/block-validation/plugins',
Copy link
Copy Markdown
Collaborator Author

@delawski delawski Apr 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since Gutenberg plugins do not render markup directly, covering them with unit tests is not practical. Instead, whenever needed, they should be covered by E2E tests.

By ignoring the plugins we ensure that the code coverage report is not negatively impacted.

@westonruter westonruter added this to the v2.1 milestone Apr 12, 2021
@westonruter
Copy link
Copy Markdown
Member

westonruter commented Apr 12, 2021

This is looking great! @amedina Any needed changes jump out at you?

Before

Post Sidebar w/o Errors Pre-Publish w/o Errors Post Sidebar w/ Errors Pre-Publish w/ Errors
Screen Shot 2021-04-11 at 23 02 59 Screen Shot 2021-04-11 at 23 03 41 Screen Shot 2021-04-11 at 23 06 23 Screen Shot 2021-04-11 at 23 06 36

After

Post Sidebar w/o Errors Pre-Publish w/o Errors Post Sidebar w/ Errors Pre-Publish w/ Errors
Screen Shot 2021-04-11 at 23 08 26 Screen Shot 2021-04-11 at 23 08 37 Screen Shot 2021-04-11 at 23 08 58 Screen Shot 2021-04-11 at 23 09 09
UI No Errors Saved State No Errors Dirty State Unreviewed Removed Reviewed Kept
Sidebar image image image image
Pre-publish image image image image

@westonruter
Copy link
Copy Markdown
Member

westonruter commented Apr 12, 2021

@delawski would you verify the behavior when AMP developer tools are turned off in the user's profile? I'll check in the morning as well.

@delawski
Copy link
Copy Markdown
Collaborator Author

delawski commented Apr 12, 2021

@delawski would you verify the behavior when AMP developer tools are turned off in the user's profile? I'll check in the morning as well.

So that's something that I haven't covered here at all. What's more, since the AMPToggle is no longer part of the block-editor entry point, no AMP toggle shows up if dev tools are disabled right now.

The new AMPToggle depends on the block validation code so that the current AMP status can be presented. This code is delivered only to users that have the dev tools enabled. What's more, this code utilizes the validation REST endpoint which is inaccessible to the users with dev tools disabled. So enabling the new amp-toggle to the non-tech-savvy users would not be a trivial task.

I think we have three ways to go:

  1. Add back the original AMPToggle to the block-editor entry point and show it only if the dev tools are disabled.
  2. Enqueue the block-validation entry point even if the dev tools are disabled and ensure only the new AMPToggle component is rendered in such a case (so extract it from the AMPDocumentStatus component and always render as a stand-alone component in each of block editor plugins).
  3. Alternatively, I guess we could grant validation endpoint access to the users with dev tools disabled and then modify the AMPDocumentStatus component so that the relevant AMP status message is presented to the non-tech-savvy users.

I guess that the last option would require some planning and UX support from @jwold. Because of this, I think it would be a good candidate for a separate issue.

In the meantime, I'm happy to update this PR so that the AMP toggle is rendered even if the dev tools are disabled.

What are your thoughts, @westonruter?

@westonruter
Copy link
Copy Markdown
Member

westonruter commented Apr 12, 2021

I'm happy to update this PR so that the AMP toggle is rendered even if the dev tools are disabled.

Yes, I think the toggle needs to be rendered still even for users with DevTools turned off. It could either go back to it's original location in the status panel, or it could be in the new AMP panel but without any status message or button to view validation. The original location may be preferred since the UI would be more discreet. You're right that eventually we'll want to have validation messages even for when DevTools is turned off, but for now we should wait until we've made sure it's good for non-technical users.

@amedina
Copy link
Copy Markdown
Member

amedina commented Apr 12, 2021

This looks great indeed! The specific text on some of the messages could be calibrated a bit, but at the moment they convey the right semantics. And the UX for presenting them to users is great.

@delawski
Copy link
Copy Markdown
Collaborator Author

delawski commented Apr 13, 2021

Yes, I think the toggle needs to be rendered still even for users with DevTools turned off.

@westonruter With 9cbd629 the AMP toggle is rendered again for users with DevTools disabled:

Screenshot 2021-04-13 at 10 50 56

westonruter
westonruter approved these changes Apr 13, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!!! 🎉

@westonruter
Copy link
Copy Markdown
Member

westonruter commented Apr 13, 2021

Fixing tests…

@westonruter westonruter enabled auto-merge April 13, 2021 22:04
@westonruter westonruter merged commit 3924519 into develop Apr 13, 2021
@westonruter westonruter deleted the update/5997-editor-sidebar-notifications branch April 13, 2021 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@westonruter westonruter westonruter approved these changes

@pierlon pierlon Awaiting requested review from pierlon

Assignees

@delawski delawski

Labels

WS:UX Work stream for UX/Front-end

Projects

None yet

Milestone

v2.1

Development

Successfully merging this pull request may close these issues.

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

3 participants

@delawski @westonruter @amedina