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

Components: Add themeable background color #45466

Merged
merged 26 commits into from
Nov 30, 2022
Merged

Components: Add themeable background color #45466

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
0d392c9
WIP
mirka Oct 27, 2022
64a2172
Tweak theme presets
mirka Oct 27, 2022
0c6a7fc
Check default accent color
mirka Oct 27, 2022
6d5977a
Fix math
mirka Oct 27, 2022
9b519d4
Refactor
mirka Oct 27, 2022
a9b184c
Fix props passing
mirka Nov 7, 2022
e6e12c6
Simplify background switcher
mirka Nov 7, 2022
8ebc4e5
Allow undefined values in validation
mirka Nov 8, 2022
ef37c95
Add unit tests
mirka Nov 8, 2022
b385609
Add description
mirka Nov 8, 2022
4e91554
Test more shade generation
mirka Nov 8, 2022
b6f567a
Check for background colors with no good foreground colors
mirka Nov 8, 2022
17986d4
Test using color variables
mirka Nov 9, 2022
2b74866
Add story for seeing generated colors
mirka Nov 9, 2022
f2fab80
Fix typo
mirka Nov 23, 2022
5db9b4b
Memoize
mirka Nov 24, 2022
90c4d51
Fixup
mirka Nov 24, 2022
48f50e6
Add more contrast validation
mirka Nov 27, 2022
9d63099
Show issues in stories
mirka Nov 29, 2022
1fc450c
Add tests
mirka Nov 29, 2022
b490e41
Update readme
mirka Nov 29, 2022
e55e47e
Update TODO comments
mirka Nov 29, 2022
a510318
Fixup
mirka Nov 30, 2022
6fd310e
Fixup test
mirka Nov 30, 2022
64a27f3
Add changelog
mirka Nov 30, 2022
f5f0d63
Disable toolbar addon for Theme stories
mirka Nov 30, 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
30 changes: 30 additions & 0 deletions packages/components/src/button/stories/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,36 @@ Default.args = {
children: 'Code is poetry',
};

export const Primary = Template.bind( {} );
Primary.args = {
...Default.args,
variant: 'primary',
};

export const Secondary = Template.bind( {} );
Secondary.args = {
...Default.args,
variant: 'secondary',
};

export const Tertiary = Template.bind( {} );
Tertiary.args = {
...Default.args,
variant: 'tertiary',
};

export const Link = Template.bind( {} );
Link.args = {
...Default.args,
variant: 'link',
};

export const IsDestructive = Template.bind( {} );
IsDestructive.args = {
...Default.args,
isDestructive: true,
};

ciampo marked this conversation as resolved.
Show resolved Hide resolved
export const Icon = Template.bind( {} );
Icon.args = {
label: 'Code is poetry',
Expand Down
31 changes: 17 additions & 14 deletions packages/components/src/button/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
box-sizing: border-box;
padding: 6px 12px;
border-radius: $radius-block-ui;
color: $gray-900;
color: $components-color-foreground;

&[aria-expanded="true"],
&:hover {
Expand Down Expand Up @@ -44,7 +44,7 @@
&.is-primary {
white-space: nowrap;
background: $components-color-accent;
color: $white;
color: $components-color-accent-inverted;
text-decoration: none;
text-shadow: none;

Expand All @@ -53,24 +53,25 @@

&:hover:not(:disabled) {
background: $components-color-accent-darker-10;
color: $white;
color: $components-color-accent-inverted;
ciampo marked this conversation as resolved.
Show resolved Hide resolved
}

&:active:not(:disabled) {
background: $components-color-accent-darker-20;
border-color: $components-color-accent-darker-20;
color: $white;
color: $components-color-accent-inverted;
}

&:focus:not(:disabled) {
box-shadow: inset 0 0 0 1px $white, 0 0 0 var(--wp-admin-border-width-focus) $components-color-accent;
box-shadow: inset 0 0 0 1px $components-color-background, 0 0 0 var(--wp-admin-border-width-focus) $components-color-accent;
}

&:disabled,
&:disabled:active:enabled,
&[aria-disabled="true"],
&[aria-disabled="true"]:enabled, // This catches a situation where a Button is aria-disabled, but not disabled.
&[aria-disabled="true"]:active:enabled {
// TODO: This needs to be accent-inverted
Copy link
Member Author

Choose a reason for hiding this comment

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

The three TODO comments in this stylesheet flag the parts that rely on Sass functions, which will not work when the underlying values are CSS variables.

There are two ways we can address these:

  1. Work with designers and see if any of the Sass function parts can be replaced with the standard gray-100 etc. colors. This is the easiest and preferred way when designing themeable components.
  2. In the Button component, access the raw theme color values (not CSS variables) via Context, and calculate the necessary colors in JS.

Copy link
Contributor

Choose a reason for hiding this comment

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

Work with designers and see if any of the Sass function parts can be replaced with the standard gray-100 etc. colors. This is the easiest and preferred way when designing themeable components.

Agreed, this would be my preferred option too — it would simplify the code and its readability too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you expand a bit on what the ask is here? Is it changing out of the main color?

I like to think that we can reach a point where we have a set of system colors with their known contrast properties in light and dark mode (the grays), and then provide a single accent color to a component, and whatever mode it's in defines what colors get applied. That is, ideally I should never have to feed more than a single color along with the dark or light mode prop to a component, and it handles the rest itself. Is that useful?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you expand a bit on what the ask is here? Is it changing out of the main color?

To be clear, I'm not asking for anything specific yet in this PR 🙂 We'll make follow-up PRs with more detail, suggest some alternatives, and ping the design team there for discussion.

As an overview of the problem though, we can no longer easily use Sass functions like this, which require the color to be predetermined:

gutenberg/packages/components/src/button/style.scss

Line 75 in 2b74866

color: rgba($white, 0.4);

☝️ This button's text color needs to be either white or black depending on the arbitrary accent color. But we can't use CSS variables as inputs to Sass functions. Things like rgba(var(--my-color), 0.4) are not possible.

gutenberg/packages/components/src/button/style.scss

Lines 131 to 132 in 2b74866

color: lighten($gray-700, 5%);
background: lighten($gray-300, 5%);

gutenberg/packages/components/src/button/style.scss

Lines 246 to 252 in 2b74866

background-image: linear-gradient(
-45deg,
darken($white, 2%) 33%,
darken($white, 12%) 33%,
darken($white, 12%) 70%,
darken($white, 2%) 70%
);

☝️ Similarly in these two examples, we can't do things like lighten(var(--gray-700), 5%).

the dark or light mode prop

I'm using the term "dark mode" for convenience, but we are currently working toward a more generic solution which allows consumers to pick their own background color. In theory this means any color, but in practice it means consumers should be able to choose their own shade of "dark" when doing "dark mode". An app may want #0a0a0a, another may want #1d1f21, etc.

color: rgba($white, 0.4);
background: $components-color-accent;
border-color: $components-color-accent;
Expand All @@ -79,15 +80,15 @@

&:focus:enabled {
box-shadow:
0 0 0 $border-width $white,
0 0 0 $border-width $components-color-background,
0 0 0 3px $components-color-accent;
}
}

&.is-busy,
&.is-busy:disabled,
&.is-busy[aria-disabled="true"] {
color: $white;
color: $components-color-accent-inverted;
background-size: 100px 100%;
// Disable reason: This function call looks nicer when each argument is on its own line.
/* stylelint-disable */
Expand All @@ -113,7 +114,7 @@
outline: 1px solid transparent;

&:active:not(:disabled) {
background: $gray-300;
background: $components-color-gray-300;
color: $components-color-accent-darker-10;
box-shadow: none;
}
Expand All @@ -126,6 +127,7 @@
&:disabled,
&[aria-disabled="true"],
&[aria-disabled="true"]:hover {
// TODO: Emotion?
ciampo marked this conversation as resolved.
Show resolved Hide resolved
color: lighten($gray-700, 5%);
background: lighten($gray-300, 5%);
transform: none;
Expand Down Expand Up @@ -222,7 +224,7 @@
}

&:not([aria-disabled="true"]):active {
color: inherit;
color: $components-color-foreground;
}

&:disabled,
Expand All @@ -238,6 +240,7 @@
animation: components-button__busy-animation 2500ms infinite linear;
opacity: 1;
background-size: 100px 100%;
// TODO: Emotion?
// Disable reason: This function call looks nicer when each argument is on its own line.
/* stylelint-disable */
background-image: linear-gradient(
Expand Down Expand Up @@ -292,19 +295,19 @@

// Toggled style.
&.is-pressed {
color: $white;
background: $gray-900;
color: $components-color-foreground-inverted;
background: $components-color-foreground;

&:focus:not(:disabled) {
box-shadow: inset 0 0 0 1px $white, 0 0 0 var(--wp-admin-border-width-focus) $components-color-accent;
box-shadow: inset 0 0 0 1px $components-color-background, 0 0 0 var(--wp-admin-border-width-focus) $components-color-accent;

// Windows High Contrast mode will show this outline, but not the box-shadow.
outline: 2px solid transparent;
}

&:hover:not(:disabled) {
color: $white;
background: $gray-900;
color: $components-color-foreground-inverted;
background: $components-color-foreground;
}
}

Expand Down
126 changes: 126 additions & 0 deletions packages/components/src/theme/color-algorithms.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
/**
* External dependencies
*/
import { colord, extend } from 'colord';
import a11yPlugin from 'colord/plugins/a11y';
import namesPlugin from 'colord/plugins/names';

/**
* WordPress dependencies
*/
import warning from '@wordpress/warning';

/**
* Internal dependencies
*/
import type { ThemeInputValues, ThemeOutputValues } from './types';
import { COLORS } from '../utils';

extend( [ namesPlugin, a11yPlugin ] );

export function generateThemeVariables(
inputs: ThemeInputValues
): ThemeOutputValues {
validateInputs( inputs );

return {
colors: {
...generateAccentDependentColors( inputs.accent ),
...generateBackgroundDependentColors( inputs.background ),
},
};
}

export function validateInputs( inputs: ThemeInputValues ) {
for ( const [ key, value ] of Object.entries( inputs ) ) {
if ( typeof value !== 'undefined' && ! colord( value ).isValid() ) {
warning(
`wp.components.Theme: "${ value }" is not a valid color value for the '${ key }' prop.`
);
}
}

if ( inputs.background ) {
if (
! colord( inputs.background ).isReadable(
inputs.accent || COLORS.ui.theme
)
) {
warning(
`wp.components.Theme: The background color provided ("${ inputs.background }") does not have sufficient contrast against the accent color ("${ inputs.accent }").`
);
}

if (
! colord( inputs.background ).isReadable(
getForegroundForColor( inputs.background )
)
) {
warning(
`wp.components.Theme: The background color provided ("${ inputs.background }") does not have sufficient contrast against the standard foreground colors.`
);
}
}
}

function generateAccentDependentColors( accent?: string ) {
if ( ! accent ) return {};

return {
accent,
accentDarker10: colord( accent ).darken( 0.1 ).toHex(),
accentDarker20: colord( accent ).darken( 0.2 ).toHex(),
accentInverted: getForegroundForColor( accent ),
};
}

function generateBackgroundDependentColors( background?: string ) {
if ( ! background ) return {};

const foreground = getForegroundForColor( background );

return {
background,
foreground,
foregroundInverted: getForegroundForColor( foreground ),
gray: generateShades( background, foreground ),
};
}

function getForegroundForColor( color: string ) {
return colord( color ).isDark() ? COLORS.white : COLORS.gray[ 900 ];
}

export function generateShades( background: string, foreground: string ) {
ciampo marked this conversation as resolved.
Show resolved Hide resolved
// How much darkness you need to add to #fff to get the COLORS.gray[n] color
const SHADES = {
100: 0.06,
200: 0.121,
300: 0.132,
400: 0.2,
600: 0.42,
700: 0.543,
800: 0.821,
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, any reason why we don't generate a gray-900 color?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the context of our color system, gray-900 is always the same as foreground. So it was redundant.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are these opacity values? Just curious on how the hex colors are calculated. This is cool stuff.

Copy link
Member Author

Choose a reason for hiding this comment

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

These are "darkness" values, i.e. the inverse of the Lightness value in HSL.

gray-100 #f0f0f0 is hsl(0, 0%, 94%) (1 - 0.94 = 0.06, etc)
gray-200 #e0e0e0 is hsl(0, 0%, 88%) and so on...

That said, we might eventually switch from HSL to LCH for internal calculations so the lightness differences are a bit more reliable than HSL.

Copy link
Contributor

@jasmussen jasmussen Dec 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Very cool, thank you! I like the sound of that, in the past I explored HSL for component theming, and the opportunities are pretty cool. That's not to say the approach I explored there is the right one — just that it's nice to be able to feed a single hue value, and have the rest refer to that.

Edit: Especially useful if it means we can retire light/dark versions of the spot color, so we have just a single spot color.

};

// Darkness of COLORS.gray[ 900 ], relative to #fff
const limit = 0.884;

const direction = colord( background ).isDark() ? 'lighten' : 'darken';
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently, gray-100 could be the brightest or the darkest color in the scale of grays, depending on whether the background color is classified as "dark" by colord.

Could this cause confusion in the consumers of the theme? My experience as a developer has taught me that usually 100 is always associated to a brighter color, and 900 to a darker.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have an alternative for that problem specifically. Though, I'm expecting that most usages won't be using these gray colors directly, but instead through more semantically named variables (e.g. border, lightBorder, secondaryText, etc).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. Let's get a feeling for it and see if we need to make any adjustments (at most we could add a line to our docs ?)


// Lightness delta between the background and foreground colors
const range =
Math.abs(
colord( background ).toHsl().l - colord( foreground ).toHsl().l
) / 100;

const result: Record< number, string > = {};

Object.entries( SHADES ).forEach( ( [ key, value ] ) => {
result[ parseInt( key ) ] = colord( background )
[ direction ]( ( value / limit ) * range )
.toHex();
} );

return result as NonNullable< ThemeOutputValues[ 'colors' ][ 'gray' ] >;
}
26 changes: 8 additions & 18 deletions packages/components/src/theme/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,10 @@
/**
* External dependencies
*/
import { colord, extend } from 'colord';
import a11yPlugin from 'colord/plugins/a11y';
import namesPlugin from 'colord/plugins/names';

/**
* Internal dependencies
*/
import type { ThemeProps } from './types';
import type { WordPressComponentProps } from '../ui/context';
import { Wrapper } from './styles';

extend( [ namesPlugin, a11yPlugin ] );
import { generateThemeVariables } from './color-algorithms';

/**
* `Theme` allows defining theme variables for components in the `@wordpress/components` package.
Expand All @@ -36,16 +28,14 @@ extend( [ namesPlugin, a11yPlugin ] );
* };
* ```
*/
function Theme( props: WordPressComponentProps< ThemeProps, 'div', true > ) {
const { accent } = props;
if ( accent && ! colord( accent ).isValid() ) {
// eslint-disable-next-line no-console
console.warn(
`wp.components.Theme: "${ accent }" is not a valid color value for the 'accent' prop.`
);
}
function Theme( {
accent,
background,
...props
}: WordPressComponentProps< ThemeProps, 'div', true > ) {
const themeVariables = generateThemeVariables( { accent, background } );
Copy link
Member Author

Choose a reason for hiding this comment

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

The next thing we should do is to take these themeVariables values and make them available to descendant components via Context. This will allow us to resolve the remaining TODO comments in Button, where Button needs to access the color values directly and compute its non-standard colors in JS.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here's a few considerations (almost a brainstorming sessions) that came to mind when thinking about using React Context:

  • are we ok exposing the variables both in React Context and as CSS variables ?
  • Which one is the source of truth?
  • Is there a scenario in which they could not be in sync ?
  • CSS vars will be accessible to DOM descendants, while React Content will be accessible to all react child components — which gives us the chance of using one or the other when styling Slot-Fill components (e.g Popover)

Copy link
Member Author

Choose a reason for hiding this comment

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

After ruminating on this, I'll backtrack — we shouldn't use React Context in combination with the original DOM-based scoping system 😅 It would cause a lot of complications like you mention.

So I guess we really need to avoid Sass color functions as much as possible, and maybe consider exposing HSL-split CSS vars (--h --s --l) for things that are absolutely necessary 😕

ciampo marked this conversation as resolved.
Show resolved Hide resolved

return <Wrapper { ...props } />;
return <Wrapper { ...themeVariables } { ...props } />;
}

export default Theme;
Loading