Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eliminate AMP validation from Classic Editor #5996

Merged
merged 3 commits into from
Mar 19, 2021

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Mar 19, 2021

Summary

AMP validation in the Classic Editor is very poor. Since the Classic Editor requires a full page reload for each save, the synchronous validation performed during that request makes saving painfully long. And then if there was a validation error in the content, the presentation of the errors is poor since the specific content that has the error(s) can't be pinpointed:

image

Also, since the Block Editor is the future and the Classic Editor is the past, we should move along. A lot of legacy technical debt can be paid off by eliminating this integration. The Classic Editor integration was not being maintained and it is doubtful it provided any value for users. If a user is using the Classic Editor, they will still be able to discover validation errors via the admin bar when viewing the post on the frontend.

This is part of #2069.

Before

Time to save: 5 seconds

before.mov

After

Time to save: 1 second

after.mov

Checklist

@westonruter westonruter added this to the v2.1 milestone Mar 19, 2021
@westonruter westonruter self-assigned this Mar 19, 2021
@westonruter westonruter force-pushed the remove/classic-editor-validation branch from 8308aaa to 59decd4 Compare March 19, 2021 06:54
Base automatically changed from feature/5304-validation-notifications to develop March 19, 2021 17:01
@westonruter westonruter force-pushed the remove/classic-editor-validation branch from 59decd4 to 7f7f447 Compare March 19, 2021 17:31
@codecov
Copy link

codecov bot commented Mar 19, 2021

Codecov Report

Merging #5996 (9a69705) into develop (bcd47bf) will decrease coverage by 0.07%.
The diff coverage is n/a.

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

@@              Coverage Diff              @@
##             develop    #5996      +/-   ##
=============================================
- Coverage      75.41%   75.34%   -0.08%     
+ Complexity      5735     5700      -35     
=============================================
  Files            218      218              
  Lines          17362    17277      -85     
=============================================
- Hits           13094    13017      -77     
+ Misses          4268     4260       -8     
Flag Coverage Δ Complexity Δ
javascript 80.05% <ø> (ø) 0.00 <ø> (ø)
php 75.14% <ø> (-0.08%) 0.00 <ø> (ø)

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

Impacted Files Coverage Δ Complexity Δ
...cludes/validation/class-amp-validation-manager.php 80.56% <ø> (-0.96%) 274.00 <0.00> (-35.00)

@westonruter westonruter marked this pull request as ready for review March 19, 2021 17:41
@github-actions
Copy link
Contributor

github-actions bot commented Mar 19, 2021

Plugin builds for fec0b8d are ready 🛎️!

@westonruter westonruter added the WS:Core Work stream for Plugin core label Mar 19, 2021
@amedina
Copy link
Member

amedina commented Mar 19, 2021

This is a great path to move forward on. Although it would seem users of the Classic Editor would have less visibility into validation issues right from the editor, actually the UX is much better this way.

Copy link
Member

@amedina amedina left a comment

Choose a reason for hiding this comment

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

This is great. Love to see all that code being cut.

Ship it.

@@ -119,6 +119,7 @@ class AMP_Validation_Manager {
*
* Keys are post IDs and values are whether the post has been re-validated.
*
* @deprecated In 2.1 the classic editor block validation was removed.
* @var bool[]
*/
public static $posts_pending_frontend_validation = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

This variable can now be removed, no? Unless it was being referenced directly by themes/plugins.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can see that two plugins are making use of this variable, but in those cases they rely on the AMP plugin as a library so removing this variable shouldn't affect them.

Copy link
Member Author

Choose a reason for hiding this comment

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

It can't be removed because it's being used by this mini plugin: https://gist.github.com/westonruter/31ac0e056b8b1278c98f8a9f548fcc1a

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see. Maybe we could add a note about that also so it isn't accidentally removed in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added

includes/validation/class-amp-validation-manager.php Outdated Show resolved Hide resolved
includes/validation/class-amp-validation-manager.php Outdated Show resolved Hide resolved
includes/validation/class-amp-validation-manager.php Outdated Show resolved Hide resolved
@westonruter westonruter merged commit a97651d into develop Mar 19, 2021
@westonruter westonruter deleted the remove/classic-editor-validation branch March 19, 2021 19:57
@westonruter westonruter removed their assignment Apr 23, 2021
@jwold jwold self-assigned this Apr 23, 2021
@jwold
Copy link
Collaborator

jwold commented Apr 23, 2021

I tried to reproduce for QA, but couldn't validate.

  1. Installed Classic editor in Standard mode
  2. On new page, in text mode, typed:

<script>document.write('Test.');</script>
<div attr="attr1">Div attribute error test</div>

  1. When viewing on frontend I could see zero errors displayed in admin bar. Not sure if it's functioning correctly.

@jwold
Copy link
Collaborator

jwold commented Apr 23, 2021

Something was wrong with my local instance of AMP. I did a clean install and validated that this issue is working. When I type an invalid entry into Classic editor, then go to frontend, I now see the error displaying in the admin bar. Passes QA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WS:Core Work stream for Plugin core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants