-
Notifications
You must be signed in to change notification settings - Fork 56
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
Conversation
@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 👍 |
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 😄 |
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): 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. |
Hey @hypest - thanks for checking it out! The error comes from And the warnings come from: We could downgrade the error into a warning, getting this result: We still get multiple warnings, but it's explicit that it's about Block validation. Alternatively, we can deactivate the red screen for errors. It will still display for Exceptions. |
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. |
Thank you @iamthomasbishop for the design specs! Note that the media icons are black. This will be easily fixed after this PR is merged: #1017 |
Perfect! Thanks for the attention to detail! |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I didn't get here in time to review, but 😍 |
Fixes #969
This PR implements
BlockInvalidWarning
onBlockHolder
to show a warning UI when the block content is invalid.gutenberg
side PR: WordPress/gutenberg#15789To test:
Update release notes:
RELEASE-NOTES.txt
.