border: 1px → $border-width #64680
Merged
border: 1px → $border-width #64680
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jameskoster
added
the
[Type] Code Quality
jameskoster
requested review from
ajitbohra,
getdave and
ellatrix
as code owners
|
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. |
jasmussen
approved these changes
Aug 21, 2024
| @@ -73,7 +73,7 @@ | |||
|
|
|||
|
|
|||
| .block-editor-global-styles-background-panel__inspector-media-replace-container { | |||
| border: 1px solid $gray-300; | |||
| border: $border-width solid $gray-300; | |||
| border-radius: 2px; | |||
There was a problem hiding this comment.
The rabbit hole is endless. Should these 2px be $radius-block-ui;? Or even the new scale, small?
There was a problem hiding this comment.
Next steps for radius is a bit tricky, we need to:
- Eliminate any hard-coded values like this
- Apply the radius scale in the editor
- Tidy up any unnecessary
border-radiusoverrides (there are quite a few in the editor)
So potential next steps would be:
- Replace any hard-coded values like this one with
$radius-block-ui - Use
$radius-block-uias a to-do list; manually go through and replace instances of it with the appropriate values from the new scale.
There was a problem hiding this comment.
Zero rush. Doesn't have to be here, or now, or you. Just wanted to mention it as I saw it.
| @@ -101,7 +101,7 @@ | |||
| } | |||
|
|
|||
| .block-editor-global-styles-background-panel__image-tools-panel-item { | |||
| border: 1px solid $gray-300; | |||
| border: $border-width solid $gray-300; | |||
| border-radius: 2px; | |||
There was a problem hiding this comment.
Same radius question here.
| @@ -197,7 +197,7 @@ | |||
| border-radius: $radius-round; | |||
| box-shadow: inset 0 0 0 $border-width rgba(0, 0, 0, 0.2); | |||
| // Show a thin outline in Windows high contrast mode, otherwise the button is invisible. | |||
| border: 1px solid transparent; | |||
There was a problem hiding this comment.
This one is fine to change, but there are many cases where there are 2 or even 3px outlines destined for high contrast mode, so it's less uniform here. In other words, this one could intentionally diverge. Either is fine, just noting.
| @@ -112,7 +112,7 @@ $components-custom-gradient-picker__padding: $grid-unit-20; // 48px container, 1 | |||
| &.is-pressed { | |||
| > svg { | |||
| background: $white; | |||
| border: 1px solid $gray-600; | |||
| border: $border-width solid $gray-600; | |||
| border-radius: 2px; | |||
| @@ -9,7 +9,7 @@ | |||
| } | |||
|
|
|||
| .editor-block-manager__disabled-blocks-count { | |||
| border: 1px solid $gray-300; | |||
| border: $border-width solid $gray-300; | |||
| border-width: 1px 0; | |||
There was a problem hiding this comment.
Probably want it here too no? I.e. border-width: $border-width 0;
| @@ -86,7 +86,7 @@ | |||
| } | |||
|
|
|||
| .copy-button__container--secondary { | |||
| border: 1px #c6c6c8; | |||
| border: $border-width #c6c6c8; | |||
There was a problem hiding this comment.
Speaking of rabbit holes, what's this color? 😱
| @@ -11,7 +11,7 @@ | |||
| box-sizing: border-box; | |||
| width: $sidebar-width; | |||
| background-color: $white; | |||
| border: 1px dotted $gray-300; | |||
| border: $border-width dotted $gray-300; | |||
| height: auto !important; // Need to override the default sidebar positionnings | |||
There was a problem hiding this comment.
I'm sorry for widening the rabbit hole! The CSS comment here both has a typo, and could use a period at the end. Something like // Necessary to override the default sidebar positioning.
| @@ -4,7 +4,7 @@ | |||
| } | |||
| background: $white; | |||
| border-radius: $radius-block-ui; | |||
| border: 1px solid $gray-900; | |||
| border: $border-width solid $gray-900; | |||
| padding: $grid-unit-15 - 1px; // Subtract the border width. | |||
There was a problem hiding this comment.
This is a place where the variable can actually be helpful in the math. You can even remove the CSS comment if you do this:
padding: $grid-unit-15 - $border-width;
| @@ -4,7 +4,7 @@ | |||
| } | |||
| background: $white; | |||
| border-radius: $radius-block-ui; | |||
| border: 1px solid $gray-900; | |||
| border: $border-width solid $gray-900; | |||
| padding: $grid-unit-15 - 1px; // Subtract the border width. | |||
| max-height: calc(100vh - 2px); // Subtract the border width (both top and bottom). | |||
There was a problem hiding this comment.
This one would be slightly tricker since we need to output the sass variable. But this should work:
calc(100vh - #{ $border-width } - #{ $border-width });
| @@ -57,7 +57,7 @@ | |||
| font-family: system-ui; | |||
| background-color: transparent; | |||
| box-sizing: border-box; | |||
| border: 1px solid $gray-700; | |||
| border: $border-width solid $gray-700; | |||
| border-radius: 3px; | |||
There was a problem hiding this comment.
Also unrelated, but I cannot help myself. Should this be $radius-small + $border-width?
There was a problem hiding this comment.
Maybe, I'm not too familiar with the legacy widget editor.
|
Thanks for the detailed review! |
|
Size Change: +32 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in e06dace. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10489890908
|
bph
pushed a commit
to bph/gutenberg
that referenced
this pull request
Aug 31, 2024
Co-authored-by: jameskoster <[email protected]> Co-authored-by: jasmussen <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Replace instances of
border: 1pxwithborder: $border-width.Why?
Code cleanliness. Using the variable eases maintenance should it ever be decided that
$border-widthshould change.Testing Instructions
$border-widthcurrently equals1px.