-
Notifications
You must be signed in to change notification settings - Fork 26
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
Update inline code padding and use border-radius variable #447
base: trunk
Are you sure you want to change the base?
Conversation
@@ -32,17 +32,17 @@ pre { | |||
padding: 20px; | |||
background-color: #f7f7f7; | |||
border: 1px solid var(--wp--preset--color--light-grey-1); | |||
border-radius: 2px; | |||
border-radius: var(--wp--custom--button--border--radius); |
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.
While this is the same value, semantically it's a different use, and I think we should add a new custom var
border-radius: var(--wp--custom--button--border--radius); | |
border-radius: var(--wp--custom--code--border--radius); |
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.
Are you suggesting adding style in the parent theme here? If so, do you think we should also include the complete styling for the code here, like the color of the code, padding, etc.? Perhaps we can ask the design team to provide a standard design for the code.
And what do you think we should call this? I reckon a callout is not a typical button or code?
"code": {
"border": {
"color": "var(--wp--preset--color--light-grey-1)",
"radius": "2px",
"style": "solid",
"width": "1px"
}
},
Or you're just referring to adding something like these two.
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.
"code": {
"border": {
"radius": "2px",
}
},
Yep exactly that. I guess it makes sense to add it to parent, even if most child themes might not use 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.
And what do you think we should call this? I reckon a callout is not a typical button or code?
Callouts are definitely different, not sure what you're asking here sorry... can you expand on 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.
Ah, yeah. I'm referring to the button
in the --wp--custom--button--border--radius
.
Since this border radius belongs to a callout, I assume we also want it to be something like --wp--custom--callout--border--radius
.
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.
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.
Ah, yeah. I'm referring to the
button
in the--wp--custom--button--border--radius
. Since this border radius belongs to a callout, I assume we also want it to be something like--wp--custom--callout--border--radius
.
Sorry I get it now. Yeah I don't think this change should be included, unless we're going to update the mu-plugins callout styles to also use a var.
Could you please give an example for testing? |
@ndiego icymi. |
@@ -633,7 +630,7 @@ pre { | |||
color: var(--wp--custom--wporg-callout--color--text); | |||
background-color: var(--wp--custom--wporg-callout--color--background); | |||
border-width: 0; | |||
border-radius: 2px; | |||
border-radius: var(--wp--custom--button--border--radius); |
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 should revert this, as it's a value copied from mu-plugins, see comment.
@@ -435,7 +432,7 @@ pre { | |||
|
|||
.wporg-developer-code-block { | |||
$half_padding: calc(var(--wp--preset--spacing--10) / 2); | |||
$border_radius: 2px; | |||
$border_radius: var(--wp--custom--code--border--radius); |
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.
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.
Sorry this has gone back and forth, but I think we should reduce the scope of this. Variablizing all 2px values doesn't make sense to me without creating more granular vars.
This is a minor tweak to inline code styles. Previously, the padding was hardcoded and did not adjust based on the size of the font. There are a few instances where inline code is used in headings.
This PR updates the left and right padding to
0.25em
. This translates to a padding increase of1px
to4px
for article copy.While adjusting the padding, I also updated all instances of
border-radius: 2px
to use the custom variable.