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

Blocks: Stop showing errors and warnings when blocks gets successfully updated from deprecated version #16862

Merged
merged 2 commits into from Aug 30, 2019

Conversation

@gziolo
Copy link
Member

commented Aug 1, 2019

Description

This was something I wanted to tackle for quite some time. I decided to finally approach it when I worked on #16794 and I encountered this issue (#16794 (comment)):

When I go to Gutenberg demo page locally I see lots of warnings on JS console. I plan to open a related PR where I will improve the feedback shared on the JS console. It clearly is wrong as is, we should only let developers know that post was properly converted and show what was before and how it is coded from now on.

Screen Shot 2019-07-29 at 17 00 52

This is how the same demo page behaves when loaded with this branch:

Screen Shot 2019-08-01 at 17 29 56

Instead of throwing scary warnings and errors, the proposed implementation only informs on Web Console that block was successfully updated to the latest version.

How has this been tested?

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@gziolo gziolo requested review from mcsf and youknowriad Aug 1, 2019

@gziolo gziolo changed the title Update/block validation warnings Blocks: Stop showing errors and warnings when we successfully update deprecated version Aug 1, 2019

@gziolo

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2019

@youknowriad and @mcsf - I'd love to hear your thoughts on the general direction proposed in this commit: 7be5763.

@gziolo gziolo changed the title Blocks: Stop showing errors and warnings when we successfully update deprecated version Blocks: Stop showing errors and warnings when blocks gets successfully updated from deprecated version Aug 1, 2019

@gziolo gziolo self-assigned this Aug 2, 2019

@youknowriad
Copy link
Contributor

left a comment

The end result is satisfying but I have some concerns about the "logger" implementation and the fact that we need a stateful object.

What if instead we just change the result of the validation functions from being a boolean with messaged logged to being an object containing a status and messages and let the caller decide what to do with these?

@gziolo

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2019

What if instead we just change the result of the validation functions from being a boolean with messaged logged to being an object containing a status and messages and let the caller decide what to do with these?

I'm not happy about that as well. This was my biggest concern from the beginning but I wanted to focus on the experience first.

I considered something similar to what you described. The biggest concern is that isValidBlockContent is part of public API so we would have to find a way to make it backward compatible for those who expect boolean as a return value. Unless we add new options object param which would alow to change the return value:

/**
 * @param {string|Object} blockTypeOrName        Block type.
 * @param {Object}        attributes             Parsed block attributes.
 * @param {string}        originalBlockContent   Original block content.
 * @param {?Object}       options                Additional config options.
 * @param {?Boolean}      options.detailedReport Return all errors and warnings.
 *
 * @return {boolean|Object} Whether block is valid. When options.detailedReport
 *                          is set to true, this function will return an object with
 *                          the list of errors or warnings and isValid property.
 */
export function isValidBlockContent( blockTypeOrName, attributes, originalBlockContent, { detailedReport = false } ) {
    const isValid = true; // or false
    if ( detailedReport ) {
        return {
            isValid,
            logs,
        };
    }
    return isValid;
}
@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

@gziolo We could use another function internally and make isValidBlockContent use that internal function and just returns the status and log the messages.

@mcsf
Copy link
Contributor

left a comment

Nice that you're working on this.

  1. I agree with what's discussed around avoiding statefulness if practical.

  2. The flow in the log can be a bit awkward depending on what is valid and invalid. Steps to repro:

2.1. Change the demo content by adding a bogus data attribute to the first DIV, like so:

diff --git a/post-content.php b/post-content.php
index 032c4ad10..7f3621ada 100644
--- a/post-content.php
+++ b/post-content.php
@@ -7,7 +7,7 @@
 
 ?>
 <!-- wp:cover {"url":"https://cldup.com/Fz-ASbo2s3.jpg","className":"alignwide"} -->
-<div class="wp-block-cover has-background-dim alignwide" style="background-image:url(https://cldup.com/Fz-ASbo2s3.jpg)"><div class="wp-block-cover__inner-container"><!-- wp:paragraph {"align":"center","placeholder":"Write title…","fontSize":"large"} -->
+<div class="wp-block-cover has-background-dim alignwide" data-foo="bar" style="background-image:url(https://cldup.com/Fz-ASbo2s3.jpg)"><div class="wp-block-cover__inner-container"><!-- wp:paragraph {"align":"center","placeholder":"Write title…","fontSize":"large"} -->
 <p style="text-align:center" class="has-large-font-size"><?php _e( 'Of Mountains &amp; Printing Presses', 'gutenberg' ); ?></p>
 <!-- /wp:paragraph --></div></div>
 <!-- /wp:cover -->

2.2. This will break the opening Cover block of the demo.

2.3. Open the demo post. Notice how the first line logged reads Block successfully updated for core/paragraph, which refers to the P tag inside the broken Cover block. The following lines are errors logged for the invalid Cover block as a whole.

2.4. The first line, about core/paragraph, should probably not be there, since in the end we didn't get a new Paragraph block in the block list, we only get an invalid Cover block.

packages/blocks/src/api/validation/logger.js Outdated Show resolved Hide resolved
@gziolo gziolo referenced this pull request Aug 13, 2019
0 of 5 tasks complete

@gziolo gziolo force-pushed the update/block-validation-warnings branch from 7be5763 to ee8519a Aug 27, 2019

@gziolo gziolo marked this pull request as ready for review Aug 27, 2019

@gziolo gziolo requested a review from ellatrix as a code owner Aug 27, 2019

@gziolo

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2019

@gziolo We could use another function internally and make isValidBlockContent use that internal function and just returns the status and log the messages.

I followed this recommendation from @youknowriad and introduced an internal method getBlockContentValidationResult.

2.3. Open the demo post. Notice how the first line logged reads Block successfully updated for core/paragraph, which refers to the P tag inside the broken Cover block. The following lines are errors logged for the invalid Cover block as a whole.

2.4. The first line, about core/paragraph, should probably not be there, since in the end we didn't get a new Paragraph block in the block list, we only get an invalid Cover block.

I can reproduce that. Well, technically speaking it's correct because we validate/migrate nested blocks first. I understand this might be confusing. I guess, we could resolve that but I don't see it as a blocker since the proposed solution is way much better than what we have as of today. Anyway, I will look into a nice fix tomorrow :)

@mcsf
mcsf approved these changes Aug 29, 2019
Copy link
Contributor

left a comment

Looks great! As you pointed out, any bits such as #16862 (comment) is something we can figure out later.

@gziolo gziolo merged commit 1232dae into master Aug 30, 2019

2 checks passed

pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details

@gziolo gziolo deleted the update/block-validation-warnings branch Aug 30, 2019

@gziolo gziolo added this to the Gutenberg 6.5 milestone Aug 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.