Skip to content
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

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from

Conversation

ndiego
Copy link
Member

@ndiego ndiego commented Dec 16, 2023

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 of 1px to 4px for article copy.

While adjusting the padding, I also updated all instances of border-radius: 2px to use the custom variable.

@ndiego ndiego added [Type] Enhancement New feature or request Redesign labels Dec 16, 2023
@ndiego ndiego added this to the Iteration 1 milestone Dec 16, 2023
@ndiego ndiego requested review from adamwoodnz and renintw December 16, 2023 17:03
@ndiego ndiego self-assigned this Dec 16, 2023
@@ -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);
Copy link
Contributor

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

Suggested change
border-radius: var(--wp--custom--button--border--radius);
border-radius: var(--wp--custom--code--border--radius);

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

The custom code block style has been added to parent. And the button has also been replaced with code here through 39fb3f1.

Results

image
image

Copy link
Contributor

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.

@renintw renintw requested a review from adamwoodnz January 22, 2024 11:41
@adamwoodnz
Copy link
Contributor

There are a few instances where inline code is used in headings.

Could you please give an example for testing?

@renintw
Copy link
Contributor

renintw commented Feb 15, 2024

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);
Copy link
Contributor

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);
Copy link
Contributor

@adamwoodnz adamwoodnz Feb 20, 2024

Choose a reason for hiding this comment

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

This isn't inline code, rather it's the container, so I don't think it should use this var. Reason being; if I were to change this variable to increase the inline code style, I don't think I'd expect these container styles to also change.

Screenshot 2024-02-20 at 3 21 02 PM

Copy link
Contributor

@adamwoodnz adamwoodnz left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Redesign [Type] Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants