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.

Sign up for GitHub

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

Global Styles Sidebar: tweak separator margin #40526

Merged
merged 2 commits into from
Apr 22, 2022
Merged

Global Styles Sidebar: tweak separator margin #40526

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4f739a2
Global Styles Sidebar: tweak separator margin and color
ciampo Apr 21, 2022
004800b
Remove separator color override
ciampo Apr 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions packages/edit-site/src/components/sidebar/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,3 +136,10 @@ $split-border-control-offset: 55px;
}
}
}

// Override the `hr` styles defined for the complementary area component
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 CardDivider styles, so that they would not get overridden by the styles of ComplementaryArea — 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 of ComplementaryArea may 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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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:

  1. 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.
  2. We remove he border-color override here, and fix the component in the same PR.

What do you think?

Copy link
Contributor Author

@ciampo ciampo Apr 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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:

  1. 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

  2. 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 of rgba(0, 0, 0, 0.1) in the color panel

  3. Review the style overrides in ComplementaryArea. Despite the changes from the points above, the separator in the Global Styles sidebar will keep using $gray-100 because of the style overrides from ComplementaryArea. 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)).

Copy link
Contributor

Choose a reason for hiding this comment

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

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 👍 👍

ciampo marked this conversation as resolved.
Show resolved Hide resolved
}