-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Fix overflow of Highlighted white-space in Code Block #70301
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
base: trunk
Are you sure you want to change the base?
Conversation
|
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @coderGtm! In case you missed it, we'd love to have you join us in our Slack community. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Added Since the Code Block internally uses the RichText Component, it has stylings of RichText applied. And the RichText uses some default styles which are defined here. These styles are applied as inline styles and take precedence over the Code Block's Another option would be to change the value of
|
| font-family: inherit; | ||
| overflow-wrap: break-word; | ||
| white-space: pre-wrap; | ||
| white-space: break-spaces !important; |
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.
I'd recommend leave the pre-wrap as a fallback and move the break-spaces into an @supports query since the property is still newer.
fabiankaegy
left a comment
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.
Thanks for the quick followup :)
After thinking about it for a bit more I have one additional request. We really should avoid using !important wherever we can. I know you explained why you had to use it here and it does make sense. But I think there is an alternative we could use here.
Let's remove the !important from the main stylesheet and then add a separate editor only stylesheet in which we can use a more specific css selector which then can override the property without the usage of !important.
This also ensures that the specificity on the frontend remains as low as possible to 3rd parties can override it easily :)
|
Hi. I understand your pov about avoiding Another solution I found right now, is to give Inline style to the Code Block after the initial Inline styles by Rich Text are applied. This will not require |
|
Hi @coderGtm , I’ve tested this PR on the frontend and block editor and can confirm the highlight no longer overflows the code block as intended. Environment
Steps to Reproduce
Expected Result Actual Result Screenshots
Block editor
cc: @fabiankaegy |
|
Hi @fabiankaegy Did you get a chance to review the changes? Is there anything else expected that I am missing? Thanks |
|
Just a ping letting reviewers know that all tests are now passing. cc: @fabiankaegy |
…berg into fix/link-rel-text
Mamaduka
left a comment
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.
@coderGtm, I just noticed that you've pushed changes on your fork's trunk`, which makes it harder to test locally.
You should always use feature/fix branches for the PRs. Do you mind extracting this?
| const SUPPORTS_BREAK_SPACES = | ||
| typeof CSS !== 'undefined' && | ||
| // eslint-disable-next-line no-undef | ||
| CSS.supports && | ||
| // eslint-disable-next-line no-undef | ||
| CSS.supports( 'white-space', 'break-spaces' ); |
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.
I think we can simplify this with optional chaining - !! CSS?.supports?.( 'white-space', 'break-spaces' );
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.
It think it is safer as the suggested expression will short-circuit and return undefined if either CSS or CSS.supports is not defined.
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.
The suggested expression will always return a Boolean, because of the !! operator.
if either CSS or CSS.supports is not defined.
Then SUPPORTS_BREAK_SPACES should be false, this is what we want, no?
| /** | ||
| * If the browser supports 'break-spaces', we can use it for better | ||
| * handling of whitespace in code blocks. Otherwise, we fall back to | ||
| * 'pre-wrap', which is more widely supported. | ||
| * | ||
| * @constant | ||
| * @type {string} | ||
| */ | ||
| const whiteSpaceStyle = SUPPORTS_BREAK_SPACES ? 'break-spaces' : 'pre-wrap'; |
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.
Do we need a comment for this line? It just described what the code is doing, which is already visible.
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.
I added it as a standard coding practice but yeah understandable as it's not needed really. Removed it.
|
@Mamaduka That is one of the reasons I want to get this PR approved and merged so I can safely update my fork's trunk 😅 I'm not sure what you mean by extracting it. Afaik once a PR is raised its branch cannot be changed, right? |
|
Thanks for the PR. The details of the implementation are well covered by other reviewers, so I'll mostly comment on expectations of the behavior of the codeblock, seeing as this is using an exciting and new to me feature of break-spaces. Which is to say, the "after" image is what I would also expect as far as the default out-of-box experience, so I think the PR/approach is reasonable. Should we choose differently at some point in the future, a block support could be considered for the code block to toggle this behavior off, if you really wanted the spaces to break out. |
|
Thanks for the input, @jasmussen! I agree, let's keep things simple for now. |





What?
Closes #69668
Fixes the highlight issue for the frontend but does not seem to be working for editor.
Why?
Currently, when text within a code block is highlighted and includes extra white space, it can overflow beyond the code box, which is not ideal. While this issue is uncommon, it negatively affects the overall UI experience when it does happen.
How?
Changed the
white-spaceproperty tobreak-spacesin thestyle.scssfile.Testing Instructions
Screenshots
Note
While this solves the issue on the frontend, it is still not solved in the editor. Despite applying the css in
style.css, it is overiden by inline styles of the Code block that addswhite-space: pre-wrap; min-width: 1px;to the element:While I am searching the source of this inline style, any guidance would be helpful.
Thanks