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

Refactoring and improving validation error handling #2285

Merged
merged 21 commits into from
May 15, 2019

Conversation

swissspidy
Copy link
Collaborator

@swissspidy swissspidy commented May 12, 2019

An attempt to reorganize the block validation and block editor scripts in order to further streamline the block editor integration.

Uses data stores to read and write necessary information, and then withSelect to ensure any validation errors are immediately shown without having to interact with the blocks.

While restructuring the code for that I stumbled upon many parts that could be simplified or even completely removed because they were not actually used at all.

Demo: https://cloudup.com/c00a-dLj555

Fixes #2127.
Iterates on #1298.

@googlebot googlebot added the cla: yes Signed the Google CLA label May 12, 2019
@swissspidy swissspidy force-pushed the fix/block-validation-handling branch 2 times, most recently from 54149d1 to dc4921d Compare May 13, 2019 18:06
@swissspidy swissspidy force-pushed the fix/block-validation-handling branch from dc4921d to f39fb77 Compare May 13, 2019 18:52
@swissspidy swissspidy changed the title [WIP] Refactoring and improving validation error handling Refactoring and improving validation error handling May 14, 2019
@swissspidy swissspidy marked this pull request as ready for review May 14, 2019 00:39
@swissspidy swissspidy requested a review from westonruter May 14, 2019 00:40
@westonruter
Copy link
Member

IT IS A THING OF BEAUTY.

@westonruter
Copy link
Member

I'm getting an error on the Validated URL screen:

image

Also getting a JS error when viewing details of a single validation error:

image

@westonruter
Copy link
Member

I got a JS error when I:

  1. Switched to Native mode.
  2. Added an AMP Timeago block to a post
  3. Switched to Transitional mode
  4. Tried to edit the post with the AMP Timeago block
TypeError: Cannot read property 'title' of undefined
    at https://wordpressdev.lndo.site/content/plugins/gutenberg/build/block-editor/index.js?ver=1557784482:55:114803
    at t.value (https://wordpressdev.lndo.site/content/plugins/gutenberg/build/block-editor/index.js?ver=1557784482:55:109706)
    at finishClassComponent (https://wordpressdev.lndo.site/content/plugins/gutenberg/vendor/react-dom.165d5c53.js:14830:31)
    at updateClassComponent (https://wordpressdev.lndo.site/content/plugins/gutenberg/vendor/react-dom.165d5c53.js:14785:24)
    at beginWork (https://wordpressdev.lndo.site/content/plugins/gutenberg/vendor/react-dom.165d5c53.js:15733:16)
    at performUnitOfWork (https://wordpressdev.lndo.site/content/plugins/gutenberg/vendor/react-dom.165d5c53.js:19401:12)
    at workLoop (https://wordpressdev.lndo.site/content/plugins/gutenberg/vendor/react-dom.165d5c53.js:19441:24)
    at renderRoot (https://wordpressdev.lndo.site/content/plugins/gutenberg/vendor/react-dom.165d5c53.js:19524:7)
    at performWorkOnRoot (https://wordpressdev.lndo.site/content/plugins/gutenberg/vendor/react-dom.165d5c53.js:20431:7)
    at performWork (https://wordpressdev.lndo.site/content/plugins/gutenberg/vendor/react-dom.165d5c53.js:20343:7)

@swissspidy
Copy link
Collaborator Author

Awesome, thanks a lot for testing! 🙂 😊 🎉

Looks like some variables haven't been properly exported. Probably happened sometime during an earlier webpack config rewrite. Should be an easy fix.

That AMP Timeago block issue looks a bit more interesting. The error message doesn't seem to reveal much.

@swissspidy
Copy link
Collaborator Author

I got a JS error when I ...

Hmm, this might have to do with the way blocks are registered and then unregistered again when not using native AMP.

I just tested this, and the timeago block was simply not visible at all (I expected a "this block is invalid" message). The crash only occurred once I tried switching to code editor.

amp-editor-blocks needs to run after amp-block-editor
@swissspidy
Copy link
Collaborator Author

I just pushed e95bc41 which fixes the issue you have had with the JS error, by basically restoring the behavior from develop.

By the way, while doing this modernization I also fixed a few bugs with the existing code. For example, after filtering error types you no longer get an AYS dialog when navigating away from the page (since one did not make any changes).

The filter:

Screenshot 2019-05-14 at 15 35 49

@westonruter
Copy link
Member

I'm still seeing the JS error for the AMP Timeago block existing in the content, when the block is not registered. But it is only happening now when hovering over the block:

image

@westonruter
Copy link
Member

westonruter commented May 14, 2019

Shouldn't it rather show this?

image

@swissspidy
Copy link
Collaborator Author

Shouldn't it rather show this?

Yes it should. Can you please test again?

@westonruter
Copy link
Member

Much better:

image

@swissspidy
Copy link
Collaborator Author

swissspidy commented May 14, 2019

According to Travis there is a test failure:

There was 1 failure:
1) Test_AMP_Post_Meta_Box::test_enqueue_block_assets

Failed asserting that true is false.

/tmp/wordpress/src/wp-content/plugins/amp/tests/test-class-amp-meta-box.php:91

However, locally all the Test_AMP_Post_Meta_Box tests pass for me.

@westonruter
Copy link
Member

It passes for me locally as well. I'll debug that.

@westonruter
Copy link
Member

@swissspidy ok, build fixed now. Feel free to merge when you're happy.

@swissspidy
Copy link
Collaborator Author

Awesome, thanks a lot!

There's one issue left that I have just noticed. amp-editor-blocks.js needs to be loaded for the story editor as well (adds some things like AMP settings to blocks), yet it should not load amp-block-editor.js (adds some unwanted things that do not apply to stories at all).

Will need to shuffle some stuff around for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Signed the Google CLA Validation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants