Skip to content

Conversation

@ndiego
Copy link
Member

@ndiego ndiego commented Feb 10, 2022

Description

The styling for the Code block has gone through many iterations. In the last PR #37818, the border styling was moved to the pre element. While this was an improvement, any Code block styles specified in theme.json were actually mapped to the code element. So if a user added border styling in theme.json, they would end up with a double border, one on pre set by WordPress theme.css and one set by the user.

In exploring solutions, the best appears to be removing the "__experimentalSelector": ".wp-block-code > code" line in block.json. Then any user styles in theme.json get applied to the pre element and override any theme.css styles as expected.

The one downside of this approach is that browsers commonly set font-family on code elements. As mentioned by @jasmussen in #37818, an easy solution to this is to simply set font-family: inherit on the code element, which will inherit any custom font styling applied to the pre element. This approach was implemented in this PR.

Testing Instructions

So now everything works as expected, but there is a strange issue that I am hoping someone might be able to help with. I was under the impression that Gutenberg block styles replace any set by WordPress core. However, in testing this PR, I noticed some "orphan" styling that is getting applied, which is coming from WordPress, not Gutenberg.

image

This is likely a separate issue, but be aware when testing this PR. You will want to unset these styles as indicated below. They only appear on the frontend.

image

Testing Steps

  1. Use Twenty Twenty-Two
  2. On a new page add a Code block and add some code to the block.
  3. View the block on the frontend, unset the styles indicated above, and confirm the block looks the same as in the Editor
  4. Open the theme.json file for the theme and add the following to the styles area
"core/code": {
    "border": {
	"color": "red",
        "radius": "4px"
	"style": "solid",
	"width": "1px"
    }
}
  1. Navigate back to the page with the code in the Editor and confirm the border is now red.
  2. View the block on the frontend, unset the styles indicated above, and confirm the block looks the same as in the Editor
  3. Got back to the Editor and manually change the border styling in the block sidebar UI.
  4. View the block on the frontend, unset the styles indicated above, and confirm the block looks the same as in the Editor

Types of changes

Bug Fix / Enhancement

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).
  • I've updated related schemas if appropriate.

@ndiego ndiego added [Type] Bug An existing feature does not function as intended [Block] Code Affects the Code Block labels Feb 10, 2022
@ndiego ndiego requested a review from ajitbohra as a code owner February 10, 2022 18:08
@ndiego ndiego self-assigned this Feb 10, 2022
@ndiego
Copy link
Member Author

ndiego commented Feb 10, 2022

@jasmussen here is the PR we discussed yesterday. I did discover some strange "orphan" CSS coming from WordPress even though Gutenberg was active? More info is provided above in the Testing Instructions. This is likely a separate issue, but a bit baffling.

@carolinan
Copy link
Contributor

Sort of related, I wonder why the pre element is used, and if it can be removed.

@jasmussen
Copy link
Contributor

Thanks for taking a stab! This is looking very good to me in terms of a singular styling interface that touches border and radius correctly:

border

Global styles also sets proper defaults in the right place:
gs

Happy to give this a green check, but I'd love a sanity check by someone with a little more depth of knowledge on Global Styles, such as @jorgefilipecosta perhaps?

Sort of related, I wonder why the pre element is used, and if it can be removed.

I wasn't able to find anything in the history, but my guess would be that the code tag is inline and would wrap unless paired with a pre wrapper. I suppose we could add some rules for that to styles.scss, but to me, the pre/code combination feels more semantic in terms of showing code at a block level.

@ndiego
Copy link
Member Author

ndiego commented Feb 11, 2022

I wasn't able to find anything in the history, but my guess would be that the code tag is inline and would wrap unless paired with a pre wrapper. I suppose we could add some rules for that to styles.scss, but to me, the pre/code combination feels more semantic in terms of showing code at a block level.

Yes, this is my understanding too.

Not that it directly impacts this PR, but I am concerned about that CSS coming from WordPress core (mentioned under the testing steps above). I cannot figure out why this is still getting generated when Gutenberg is active. I was under the impression that the block styling in Gutenberg get rendered instead of those in WordPress core when the plugin is active, but I could be wrong. 🤔

@jasmussen
Copy link
Contributor

CC: @oandregal, for the question around those styles in the testing steps. If you have time to look, thank you!

@oandregal
Copy link
Member

oandregal commented Feb 14, 2022

for the question around those styles in the testing steps. If you have time to look, thank you!

I've taken a look and this is what I see: it only happens when the should_load_separate_core_block_assets filter is true (which is true by default for block themes). I've tried switching it off by doing add_filter( 'should_load_separate_core_block_assets', '__return_false' ); in the functions.php of the theme and styles work as expected.

It sounds like either the generation of the block embedded stylesheets is not working as expected or someone is hooking more content into that. I couldn't find in a quick look what's happening. cc @aristath in case it rings any bell or you can help debug this more quickly than I am (I won't be able to look for a couple of days).

It sounds like it's an issue for more blocks than the code block. This is what I've found:

  • code block in wp-block-code-inline-css:
/* THIS COMES FROM STYLE.SCSS AND IS MINIFIED */
.wp-block-code code{display:block;overflow-wrap:break-word;white-space:pre-wrap}

/* THIS COMES FROM THEME.SCSS AND IS MINIFIED */
.wp-block-code{border:1px solid #ccc;border-radius:4px}.wp-block-code>code{font-family:Menlo,Consolas,monaco,monospace;padding:.8em 1em}

/* WHERE DOES THIS COME FROM? IT IS NOT MINIFIED */
.wp-block-code > code {
  font-family: Menlo, Consolas, monaco, monospace;
  color: #1e1e1e;
  padding: 0.8em 1em;
  border: 1px solid #ddd;
  border-radius: 4px;
}
  • group block in wp-block-group-inline-css:
/* THIS COMES FROM STYLE.SCSS AND IT's MINIFIED */
.wp-block-group{box-sizing:border-box}

/* THIS COMES FROM THEME.SCSS AND IT'S MINIFIED*/
:where(.wp-block-group.has-background){padding:1.25em 2.375em}

/* WHERE DOES THIS COME FROM? IT'S NOT MINIFIED. */
.wp-block-group:where(.has-background) {
  padding: 1.25em 2.375em;
}
  • separator block in wp-block-separator-inline-css:
/* WHERE DOES THIS COME FROM? */
@charset "UTF-8";

/* COMES FROM STYLE.SCSS and IT's MINIFIED */
.wp-block-separator{border-bottom:1px solid;border-top:1px solid}.wp-block-separator.is-style-dots{background:none!important;border:none;height:auto;line-height:1;text-align:center}.wp-block-separator.is-style-dots:before{color:currentColor;content:"···";font-family:serif;font-size:1.5em;letter-spacing:2em;padding-left:2em}

/* COMES FROM THEME.SCSS and IT'S MINIFIED */
.wp-block-separator{border:none;border-bottom:2px solid;margin-left:auto;margin-right:auto;opacity:.4}.wp-block-separator:not(.is-style-wide):not(.is-style-dots){width:100px}.wp-block-separator.has-background:not(.is-style-dots){border-bottom:none;height:1px}.wp-block-separator.has-background:not(.is-style-wide):not(.is-style-dots){height:2px}

/* WHERE DOES THIS COME FROM? IT'S NOT MINIFIED */
.wp-block-separator {
  border: none;
  border-bottom: 2px solid currentColor;
  margin-left: auto;
  margin-right: auto;
  opacity: 0.4;
}
.wp-block-separator:not(.is-style-wide):not(.is-style-dots) {
  width: 100px;
}
.wp-block-separator.has-background:not(.is-style-dots) {
  border-bottom: none;
  height: 1px;
}
.wp-block-separator.has-background:not(.is-style-wide):not(.is-style-dots) {
  height: 2px;
}

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change here seems to be in a good direction 👍
I think we should just address https://github.com/WordPress/gutenberg/pull/38712/files#r806088001 first.

@ndiego
Copy link
Member Author

ndiego commented Feb 14, 2022

This change here seems to be in a good direction 👍
I think we should just address https://github.com/WordPress/gutenberg/pull/38712/files#r806088001 first.

Good catch @jorgefilipecosta, I will fix this later today.

It sounds like it's an issue for more blocks than the code block.

Very interesting, thanks for taking the time to explore this. Yeah, I cannot seem to figure it out, but I am very new to this area of Gutenberg 😅

@ndiego ndiego force-pushed the try/restructure-code-block-styling branch from 4fab48a to 90bab97 Compare February 23, 2022 17:32
@ndiego
Copy link
Member Author

ndiego commented Feb 23, 2022

I've taken a look and this is what I see: it only happens when the should_load_separate_core_block_assets filter is true (which is true by default for block themes). I've tried switching it of by doing add_filter( 'should_load_separate_core_block_assets', '__return_false' ); in the functions.php of the theme and styles work as expected.

I have been able to replicate this.

It sounds like either the generation of the block embedded stylesheets is not working as expected or someone is hooking more content into that. I couldn't find in a quick look what's happening. cc @aristath in case it rings any bell or you can help debug this more quickly than I am (I won't be able to look for a couple of days).

@aristath, @oandregal looping back on this issue regarding mysterious CSS getting injected. It seems to be separate, would you like me to add a separate issue and we can track there? I am at a loss how to fix it, but I can create the issue 😅

@jorgefilipecosta
Copy link
Member

I'm merging this PR it seems the other points remaining should be tracked separately.

@jorgefilipecosta jorgefilipecosta merged commit 13392ba into WordPress:trunk Feb 24, 2022
@github-actions github-actions bot added this to the Gutenberg 12.8 milestone Feb 24, 2022
@ndiego ndiego deleted the try/restructure-code-block-styling branch February 24, 2022 17:59
@oandregal
Copy link
Member

looping back on this issue regarding mysterious CSS getting injected. It seems to be separate, would you like me to add a separate issue and we can track there? I am at a loss how to fix it, but I can create the issue sweat_smile

I went ahead and created the issue #39393

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Block] Code Affects the Code Block [Type] Bug An existing feature does not function as intended

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants