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

UI for invalid block content #1021

Merged
merged 12 commits into from May 26, 2019

Conversation

@etoledom
Copy link
Contributor

commented May 23, 2019

Fixes #969

This PR implements BlockInvalidWarning on BlockHolder to show a warning UI when the block content is invalid.

gutenberg side PR: WordPress/gutenberg#15789

IMG_1853

To test:

  • Run the example app.
  • Display on gutenberg the HTML content from here
  • You will see a red screen (since there is invalid content).
  • Dismiss the red screen.
  • Check that the invalid content UI is showing for all blocks.

Update release notes:

  • If there are user facing changes, I have added an item to RELEASE-NOTES.txt.

@etoledom etoledom added this to the v1.6 milestone May 23, 2019

@etoledom etoledom self-assigned this May 23, 2019

@etoledom etoledom requested review from hypest and koke May 23, 2019

@etoledom etoledom marked this pull request as ready for review May 23, 2019

@etoledom

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2019

@iamthomasbishop - I decided by option 1 from #969 (comment) since it's the simplest to implement for now.

I wasn't able to implement the exact style since I didn't found it on Zeplin, but I'm happy to update for any style change request 👍

@iamthomasbishop

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

Already looks really close design-wise, nice work @etoledom! And I figured option 1 would be the easiest, good call.

The design is the same as the inner part of the Unsupported/Placeholder block, but with an added description instead of the placeholder action button. I will drop you the link directly to the component in Zeplin via DM 😄

@hypest

This comment has been minimized.

Copy link
Contributor

commented May 24, 2019

Looks good @etoledom ! 🎉

I'm only concerned about the combination of UI + red screen + yellow warnings I'm seeing with this. The UI part is fine and makes sense but, I'm a bit annoyed by the red screen (since we are already handling the error). We've discussed this over chat of course so not going to go deeper into this part. What it's not clear to me is what are the warnings popping up and whether, as a developer, I will need to dive into those to investigate when I see them occuring in the demo app. Here's a screenshot from the demo app with the test content (as described in the PR description):

screenshot-1558686843742

So, if the warnings there are expected, can we find a way to consolidate with the red-screen too? It's too verbose to have so many different indications of the error (redscreen + yellow message) if they actually stem from the same error.

@etoledom

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

Hey @hypest - thanks for checking it out!

The error comes from

https://github.com/WordPress/gutenberg/blob/68030994aedb67f448acb148af989d4fdb871e87/packages/blocks/src/api/validation.js#L641

And the warnings come from:

https://github.com/WordPress/gutenberg/blob/68030994aedb67f448acb148af989d4fdb871e87/packages/blocks/src/api/validation.js#L609


We could downgrade the error into a warning, getting this result:

IMG_1862

We still get multiple warnings, but it's explicit that it's about Block validation.
If we go this way, I'd prefer to do the change in a separated PR, so we can focus the conversation and feedback about that change in particular, and also it won't delay the completion of this PR.


Alternatively, we can deactivate the red screen for errors. It will still display for Exceptions.
The downside is that we won't have visual feedback of errors anymore, just in the JS console. I'd prefer to not go on this direction.

@hypest

This comment has been minimized.

Copy link
Contributor

commented May 24, 2019

Happy to have any improvement on this one in a separate PR @etoledom , no prob 👍 .

The yellow warning was something that I think we didn't discuss over chat when we talked about the redscreen, so it took me by surprise... not trying to bring the redscreen conversation up again.

@pinarol pinarol referenced this pull request May 24, 2019
1 of 1 task complete
@etoledom

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

Thank you @iamthomasbishop for the design specs!
I updated applying them. This is the final result:

invalid

Note that the media icons are black. This will be easily fixed after this PR is merged: #1017

@iamthomasbishop

This comment has been minimized.

Copy link
Contributor

commented May 24, 2019

Perfect! Thanks for the attention to detail! :shipit:

@hypest

This comment has been minimized.

Copy link
Contributor

commented May 26, 2019

The failing Android device tests seem to fail due to some focus issues and are not related with the code and also happen on current develop (cdf1453) so, will ignore the failure.

@hypest

hypest approved these changes May 26, 2019

Copy link
Contributor

left a comment

LGTM @etoledom , nice work!

Any improvements can happen on follow up PRs and, as you proposed over chat, I took over the PR to update and merge it.

@hypest

hypest approved these changes May 26, 2019

Copy link
Contributor

left a comment

LGTM!

@hypest hypest merged commit 0533baf into develop May 26, 2019

5 of 6 checks passed

ci/circleci: Test Android on Device Your tests failed on CircleCI
Details
Peril All green. Yay.
Details
ci/circleci: Check Correctness Your tests passed on CircleCI!
Details
ci/circleci: Test Android Your tests passed on CircleCI!
Details
ci/circleci: Test iOS Your tests passed on CircleCI!
Details
ci/circleci: Test iOS on Device Your tests passed on CircleCI!
Details

@hypest hypest deleted the issue/969-ui-for-invalid-block branch May 26, 2019

@koke

This comment has been minimized.

Copy link
Member

commented May 27, 2019

I didn't get here in time to review, but 😍

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