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

Try custom properties for admin color schemes #21999

Closed
wants to merge 6 commits into from

Conversation

ryelle
Copy link
Contributor

@ryelle ryelle commented Apr 30, 2020

Description

Currently, the admin color schemes are being added by creating extra rules for every element that uses a theme color. This is creating some unexpected specificity issues, like the open sidebar panel headers & post title focus ring:

ecto-before

It's also difficult to extend the color schemes to add support for additional schemes.

This PR updates the postcss-themes plugin to generate custom properties (css variables) using the passed-in themes from base-styles. This fixes the specificity issue, since we only need one element rule to add the color, while the color itself is changed based on admin-scheme-* class.

This also adds 5 colors on either "side" of each variable, for lighter or darker shades. This is because the postcss-color-function plugin does not work on custom properties, and we use tint/shade to generate buttons with enough contrast, hover states, etc. I'm not sure if this is the best (or even a good) solution here.

This does still need an IE fallback. The most straightforward answer would be to use something like postcss-custom-properties to inject the default theme values as fallbacks, but then we wouldn't have color schemes on IE.

Screenshots

Ectoplasm theme:
ecto-after

The primary color is still used as text color sometimes, but that matches the default theme:

- GB 8.0 default theme - - this branch, coffee theme -
Screen Shot 2020-04-30 at 11 30 54 AM coffee-inserter

Types of changes

Not sure 🙂 This shouldn't break anything, but it is a total revamp of what the postcss-theme plugin generates.

How has this been tested?

Tested GB locally to see that everything used the new custom properties, and no styles were being unexpectedly overriden. Added in an extra color scheme by creating another file with custom properties defined.

Still need to test using postcss-themes on an external project.

@ryelle ryelle added [Type] Build Tooling Issues or PRs related to build tooling Needs Technical Feedback Needs testing from a developer perspective. CSS Styling Related to editor and front end styles, CSS-specific issues. labels Apr 30, 2020
@ryelle ryelle self-assigned this Apr 30, 2020
@@ -94,7 +94,7 @@ $input-size: 300px;

&:focus,
&.is-selected {
background: color(theme(primary) shade(15%));
background: theme(primary, shade-20);
Copy link
Contributor

Choose a reason for hiding this comment

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

@ryelle Making sure I understand what this is doing - is the theme function here equivalent to calling var( --color-primary--shade-20 )?

Copy link
Contributor

Choose a reason for hiding this comment

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

If yes, then with the "additional level of abstraction" I've mentioned, this would be var( --block-editor-url-input__suggestion-focus-background ) and a the --color-primary--shade-20 assigned in body.theme-name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, theme generates the var(…) code. Just to clarify, with the abstraction layer, would each theme be able to control both tokens & assignments, like this?

body.theme-red {
    --color-primary: red;
    --color-primary--shade-20: darkred;

    --block-editor-url-input__suggestion-focus-background: var( --color-primary );
}

body.theme-blue {
    --color-primary: blue;
    --color-primary--shade-20: darkblue;

    --block-editor-url-input__suggestion-focus-background: var( --color-primary--shade-20 )
}

Copy link
Contributor

Choose a reason for hiding this comment

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

My first thought is no, only tokens - controlling the assignments would be potentially too flexible and it could become a mess dealing with specific logic for color contrast. I'm not sure on that though!

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe for specific themes, as needed - but then again, a principle in using the tokens approach is to have restrictions and decisions like that already made. I feel like an idea workflow would be:

  1. Set base token values and tweak tint and shade assignments (we might want those to have names like lightest to darkest instead of numbers
  2. Pass some sort of test that checks color contrast ratios in the theme

Copy link
Contributor

@laras126 laras126 May 21, 2020

Choose a reason for hiding this comment

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

So it would be something like:

{
    themes: {
		default: {
			primary: '#f00',
			secondary: '#f00',
			accent: '#f00',
			error: '#f00',
			success: '#f00',
			tints: {
				lightest: '95',
				lighter: '90',
				light: '72',
				dark: '40',
				darker: '20',
				darkest: '5'
			},
			shades: {
				shallowest: '#',
				shallower: '#',
				shallow: '#',
				deep: '#',
				deeper: '#',
				deepest: '#'
			}
		},
		blue: {
			primary: '#f00',
			secondary: '#f00',
			accent: '#f00',
			error: '#f00',
			success: '#f00',
			tints: {
				lightest: '99',
				lighter: '82',
				light: '72',
				dark: '30',
				darker: '21',
				darkest: '8'
			},
			shades: {
				shallowest: '#',
				shallower: '#',
				shallow: '#',
				deep: '#',
				deeper: '#',
				deepest: '#'
			}
		}
	},
}

(where the '#' is a descending value for shades, varying for each theme, and the color values are different colors)

Copy link
Contributor

@laras126 laras126 May 21, 2020

Choose a reason for hiding this comment

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

Here is how that might be attached to the UI-named abstraction - the PostCSS plugin would generate those custom properties for us, then we apply them like this in the stylesheet:

body.theme-blue {
	// all the custom properties generated based on above spec for blue
}

// Not sure what this is exactly, but we to attach the colors to the UI-named properties
.some-other-context {
	--button-background-color: var( --color-primary--shade-darkest );
	--button-text-color: var( --color-primary--shade-lightest );
	--button-focus-background-color: var( --color-secondary--shade-darkest );
	--button-focus-text-color: var( --color-secondary--shade-lightest );
}

.components-button {
	background-color: var( --button-background-color );
	color: var( --button-text-color );

	// state bindings for the colors -
	// Fallbacks are perhaps in a separate stylesheet / only in the build
	&:focus {
		--button-background-color: var( --button-focus-background-color );
		--button-text-color: var( --button-focus-text-color );
	}
}

@laras126
Copy link
Contributor

laras126 commented May 14, 2020

Hi @ryelle! I really like the idea of defining a set of base colors, and generating tints and shades via the Post CSS themes. My comment here) is an example of naming the custom properties according to the UI element, and assigning the actual color to that (the additional level of abstraction I mentioned here). Would remove the need for the theme function syntax?

@ryelle
Copy link
Contributor Author

ryelle commented May 14, 2020

Would remove the need for the theme function syntax?

Well, I stuck with the theme function since it's a process that's already in place. Any move to custom properties could remove the theme syntax, since we could just text-replace in all the source CSS.

I think if we move away from the theme function in gutenberg, we'll probably want to deprecate(?) this postcss plugin & create a new one for generating the shade/tint variables, if that's the path forward.

@ryelle
Copy link
Contributor Author

ryelle commented Jun 24, 2020

Closing this PR/experiment since #23048 was merged.

@ryelle ryelle closed this Jun 24, 2020
@oandregal oandregal deleted the try/themes-custom-properties branch June 25, 2020 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CSS Styling Related to editor and front end styles, CSS-specific issues. Needs Technical Feedback Needs testing from a developer perspective. [Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants