Add validation message to custom CSS input #47132
Conversation
kevin940726
added
[Type] Enhancement
kevin940726
force-pushed
the
add/custom-css-validation
branch
from
d5f6a0b to
0405fab
Compare
|
Size Change: +1.1 kB (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
| <Notice | ||
| status="warning" | ||
| onRemove={ () => setCSSError( null ) } | ||
| politeness="polite" |
There was a problem hiding this comment.
I'm not sure if this is acceptable TBH. Is it too disturbing?
There was a problem hiding this comment.
It might be a good idea to get some design feedback on this.
Recently we replaced all notices inside block placeholders with snackbars. Maybe we can also do the same here, Snackbar + red outline.
cc @jasmussen
There was a problem hiding this comment.
Thanks for the ping. I see in this branch a notice below the input:
Indeed, that notice becomes a bit noisy. I wonder if we can do what Codepen does, which is to show a small exclamation mark in the corner:
It could be something like this:
Focusing or hovering this icon would show the error message.
If such an icon+tooltip isn't feasible, a snackbar could work, but in this case I think positional contextuality can be useful.
I realize that actually highlighting the problematic line is beyond the scope of this PR, but it would pair nicely.
Speaking of beyond the scope of this PR, it would be nice to put the "Read more about CSS" link into an ellipsis menu, and allow the CSS panel to flex all the way to the bottom, as in the original mockups.
There was a problem hiding this comment.
Thank you, Joen.
I like the idea. Syntax highlighting can potentially absorb the error notice role in the future.
Speaking of beyond the scope of this PR, it would be nice to put the "Read more about CSS" link into an ellipsis menu, and allow the CSS panel to flex all the way to the bottom, as in the original mockups.
We should probably create a tracking issue for the remaining items. I think @glendaviesnz is already working on the list.
There was a problem hiding this comment.
For now I updated #30142, let me know if that works. The original design still feels valid to me.
|
Flaky tests detected in 7d78375. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4054681593
|
| ); | ||
| setCSSError( | ||
| transformed === null ? __( 'Error while parsing the CSS.' ) : null | ||
| ); |
There was a problem hiding this comment.
Maybe we should move the error handling onBlur event? How helpful is the error message while a user is typing?
There was a problem hiding this comment.
Yeh, while typing there will be lots of invalid states. I have moved it to onBlur, and only runs in onChange if there was an existing error so that it gets cleared once the user fixes the problem.
|
Thanks for the work on this @kevin940726. I have updated it to only run onBlur as suggested by @Mamaduka, and also to run again with onChange only if there is an existing error so that it gets cleared when the user updates it. @jasmussen with the error only appearing with onBlur I think this might put this in a state that is suitable for the 6.2 release. The plan is to integrate a 3rd party lib that will provide inline linting, etc. soon after 6.2 release which will remove the need for this error message anyway, so maybe not worth exploring the custom icon for this - what do you think? |
glendaviesnz
force-pushed
the
add/custom-css-validation
branch
from
39b3c87 to
226002e
Compare
To be honest I think it's more important that we remove or move the "Read more about CSS" link, and make the input field go full-height, than any validation. CSS on its own is complicated, and needs all the real-estate available to author. If we need a validation icon, we can use the (i):
Otherwise, here's a new warning icon:
|
@jasmussen this proved more challenging than expected, due to the fact that there is a second collapsing box that appears if the theme includes its own custom CSS in theme.json, and the fact there were quite a few wrappers between the top level panel with no flex or height settings. Anyway, after adding flex all the way down there is a working PR for this here. It is quite different to the initial intent of this PR so thought it was better to handle it separately. |
glendaviesnz
force-pushed
the
add/custom-css-validation
branch
from
4c24d08 to
833bc36
Compare
|
@kevin940726, @jasmussen I have switched this to use an icon with tooltip and only onBlur: error.mp4Do you think this is ok for 6.2 to at least give some indication of why the changes may not be showing in the canvas, and we can follow up with inline code linting post 6.2? |
|
Sounds good to me! |
|
This works well, the only thing I've noticed that would be nice to fix is that the error doesn't clear when the css field is empty: |
|
Looks good. Probably not for this PR, but we should look at this panel header:
What are the steps to take for this panel to show up? Does it always show up or is it related to a theme switch? Trying to understand if there's a better term we can use, and whether it needs to be collapsible or not. And if we do need for it to be a panel like this, it should go edge to edge so the text lines up, and use sentence case like other panels:
|
|
@talldan pushed the fix for error clearing. |
@jasmussen If a theme has included its own custom CSS in the theme.json Happy for you to throw up some suggested design changes for this as a separate issue. |
|
Noting that I don't see this in WordPress 6.2 Beta 2. Is that intentional @talldan @kevin940726 ? |
|
@annezazu, the feature shipped with 15.1, so it should’ve been included since beta1. |
|
I just checked and it is working for me on 6.2-beta2-55340-src: error-alert.mp4@annezazu - you won't see the error icon bottom right until you leave the input box - but let me know if you are still not seeing it even after the input box loses focus. |
What?
Part of #30142. Add CSS validation message to custom CSS input. Currently only an error message
Error while parsing the CSS.will be shown, there's no debug info.Why?
For users to know why their CSS isn't being applied to the preview.
How?
This PR isn't ideal but it does the job for now. The challenge is how to move the validation/transformation to the input. Currently, all the transformation is done in
<EditorStyles>, which is in a different package, and it will be too late to display error messages there. This PR currently does double-transform, and we should fix that in the future.While testing this PR, I found out that the transformation library we use (
reworkcss) is at least 4 years ago and has many bugs. For instance,body{background:redwill throw an error while it should be valid syntax. On the other hand,body{backgroundd:red;}doesn't throw an error while it should be invalid. We should work on replacing/updating it in the future.Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
Kapture.2023-01-13.at.11.44.48.mp4