Global Styles Sidebar: tweak separator margin #40526
Merged
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
ciampo
added
[Type] Enhancement
|
Size Change: -1 B (0%) Total Size: 1.23 MB
ℹ️ View Unchanged
|
jasmussen
approved these changes
Apr 22, 2022
| @@ -136,3 +136,10 @@ $split-border-control-offset: 55px; | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| // Override the `hr` styles defined for the complementary area component | |||
There was a problem hiding this comment.
Short and sweet. Is this meant to be a temporary override to a point where the HR can be customized on the component level? If yes, then we might want to add a comment to that effect.
There was a problem hiding this comment.
Not really — the Global Styles Sidebar is already using CardDivider for this separator. The issue is that the sidebar internally uses a component from the @wordpress/interface package called ComplementaryArea, which introduces some style overrides, including this one on all its children hr.
A few solutions that come to mind:
- Artificially increase the specificity of the
CardDividerstyles, so that they would not get overridden by the styles ofComplementaryArea— although I wouldn't recommend going down this route, it's a hacky approach that can cause a "race for specificity" in other parts of the repo; - I guess the better solution would be to somehow rework how separators are used / styled in
ComplementaryArea, although it may be a difficult task given how widespread the usage ofComplementaryAreamay be.
Basically, the problem is that separators under ComplementaryArea are not encapsulated components, but are instead styles with some "global" css.
| // from the `@wordpress/interface` package. | ||
| .edit-site-global-styles-sidebar hr { | ||
| margin: 0; | ||
| border-color: rgba(0, 0, 0, 0.1); |
There was a problem hiding this comment.
Why do we need to override the if it's mainly the margin that need zeroing out?
While diving in here, I also noticed that the previous color is wrong. It outputs #f0f0f0 (which maps to $gray-100), but it should have been #e0e0e0 ($gray-200) like the other panel borders.
Depending on your thoughts here (maybe there's a reason you used opacity), I could imagine two paths forward:
- We remove the border-color override here, and accept the wrong color coming from the component directly, and then update the component to use the correct color (#e0e0e0) in a separate PR.
- We remove he border-color override here, and fix the component in the same PR.
What do you think?
There was a problem hiding this comment.
maybe there's a reason you used opacity
The separator in the Global Styles sidebar is rendered as a CardDivider, which uses rgba(0, 0, 0, 0.1) (as per the config) as the color of its border.
The style overrides introduced by ComplementaryArea changed the separator color to $gray-100, and therefore I introduced this rule to bring the separator color back to how the original component was intended to be.
I could imagine two paths forward:
I will go ahead and drop this color override from this PR. As for the next steps, there will be a few separate fronts to work on:
-
Review separator and border colors of
@wordpress/components. We can definitely look at reviewing and updating the color used by components in@wordpress/components(e.g. card, surface, datepicker...). Although I wasn't involved in the conversation at the time, if I had to guess that color is currently using transparency to make it blend better in case the background color is not white -
Review the
rgba(0, 0, 0, 0.1)color across Gutenberg. Depending on the results of the previous step, we should also review the remaining usage ofrgba(0, 0, 0, 0.1)in the color panel -
Review the style overrides in
ComplementaryArea. Despite the changes from the points above, the separator in the Global Styles sidebar will keep using$gray-100because of the style overrides fromComplementaryArea. We could start by changing the color used by these style overrides, or we could work on a more structured solution (see Global Styles Sidebar: tweak separator margin #40526 (comment)).
There was a problem hiding this comment.
Ah, excellent context. It does sound like we'll want to fix that at the component level.
At the moment we have these shades of gray for most of the UI:
$gray-900: #1e1e1e;
$gray-800: #2f2f2f;
$gray-700: #757575; // Meets 4.6:1 text contrast against white.
$gray-600: #949494; // Meets 3:1 UI or large text contrast against white.
$gray-400: #ccc;
$gray-300: #ddd; // Used for most borders.
$gray-200: #e0e0e0; // Used sparingly for light borders.
$gray-100: #f0f0f0; // Used for light gray backgrounds.
In this case, the 0.1 opacity divider actually matches $gray-100, but it ideally should match $gray-200 as those are used on panels and general UI.
I understand there are challenges with making the components work on a dark background, and that it can therefore make sense to use more agnostic colors that aren't optimized for a white background, perhaps even opacities. I trust that'll be taken into account 👍 👍
mirka
approved these changes
Apr 22, 2022
ciampo
changed the title
|
I removed the border color override, this is how the sidebar looks after this change:
I will wait for CI to go green and merge. |
This was referenced Apr 22, 2022
57 tasks
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?
Tweak the styles of the separator used in the root screen of the Global Styles Sidebar, removing its vertical margin and enforcing its expected color.
Why?
This is part of a series of tweaks aimed at polishing the Global Styles Sidebar — see #38934 for more details.
How?
The changes in this PR mainly override the margin styles that the
ComplementaryAreacomponent applies to all childrenhr. In particular, the margin has been changed to be0.The idea is that any vertical spacing around the separator may be sufficiently provided by the padding of the sibling components.
Testing Instructions
Screenshots or screencast