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

ColorPalette: Use computed color when css variable is passed to ColorPicker #47181

Merged
merged 3 commits into from
Jan 18, 2023

Conversation

t-hamano
Copy link
Contributor

@t-hamano t-hamano commented Jan 16, 2023

Closes #46492

What?

This PR passes the computed color to the ColorPicker component if the value of the ColorPalette component is a CSS variable. This will ensure that the correct color value is set in the input field when the custom color picker is opened.

Why?

The ColorPicker component doesn't know what actual color the CSS variable points to. Therefore, the color input field of the ColorPicker component displays #000000 as a fallback.

How?

I used the following approach to pass the computed value to the ColorPicker component:

Testing Instructions

In the block editor

Enable Empty Theme and update theme.json as follows:

/test/emptytheme/theme.json
{
	"$schema": "https://schemas.wp.org/trunk/theme.json",
	"version": 2,
	"settings": {
		"appearanceTools": true,
		"layout": {
			"contentSize": "840px",
			"wideSize": "1100px"
		},
		"color": {
			"palette": [
				{
					"name": "Normal Color",
					"slug": "normal",
					"color": "#378C60"
				},
				{
					"name": "Variable Color (Hex)",
					"slug": "hex",
					"color": "var(--wp--custom--hex)"
				},
				{
					"name": "Variable Color (RGB)",
					"slug": "rgb",
					"color": "var(--wp--custom--rgb)"
				},
				{
					"name": "Variable Color (RGBA)",
					"slug": "rgba",
					"color": "var(--wp--custom--rgba)"
				}
			]
		},
		"custom": {
			"hex": "#e42c64",
			"rgb": "rgb(45,36,138)",
			"rgba": "rgba(45,36,138, 0.5)"
		}
	}
}

The definition of the settings.custom property creates the following CSS variable, which is referenced as the value of the settings.color.palette:

--wp--custom--hex: #e42c64;
--wp--custom--rgb: rgb(45,36,138);
--wp--custom--rgba: rgba(45,36,138, 0.5);
  • Inserts a block that supports the color.
  • Open the Text Color or Background Color menu and click on one of the palettes in the THEME area.
  • When you open the custom color palette, confirm that the input field is set to the value calculated from the CSS variable.

In the storybook

  • Build Storybook and access "ColorPalette" > "CSS Variables".
  • Select one of the palettes, open the color picker, and verify that the input field is set to the calculated value.

Screenshots or screencast

2b2ac6156dbe07a7e5974fcbedb596a8.mp4

@t-hamano t-hamano self-assigned this Jan 16, 2023
@t-hamano t-hamano added [Package] Components /packages/components Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Jan 16, 2023
@t-hamano t-hamano changed the title ColorPalette: Use computed color when css variable is passed to ColorPicker ColorPalette: Use computed color when css variable is passed to ColorPicker Jan 16, 2023
@github-actions
Copy link

Flaky tests detected in 8302689.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/3930044210
📝 Reported issues:

@t-hamano t-hamano marked this pull request as ready for review January 16, 2023 12:31
@t-hamano t-hamano requested a review from ajitbohra as a code owner January 16, 2023 12:31
@t-hamano t-hamano requested review from mirka and ciampo and removed request for ajitbohra January 16, 2023 12:31
@mirka mirka linked an issue Jan 17, 2023 that may be closed by this pull request
Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

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

Ooh, thank you for the fix!

I confirmed this is working in Chrome/Firefox/Safari, and it should stay that way given that the spec says it returns "resolved values" 🤞

Can we also fix the other issue (#41459) in this PR or are you planning to do it separately?

style={
showTransparentBackground( value )
? { color: '#000' }
: {
background: value,
color:
colordColor.contrast() >
colordColor.contrast( '#000' )
? '#fff'
: '#000',
}
}

Comment on lines 217 to 221
const getComputedBackgroundColorValueFromRef = (
value: string | undefined,
ref: RefObject< HTMLElement > | null
) => {
let computedValue = value;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: The naming is kind of misleading to me. It only returns a "computed value from ref" when the value is a CSS var. Maybe something like normalizeColorValue() or getValueForColord()? And we can probably remove the let computedValue if we do an early return like:

if ( ! currentValueIsCssVariable ) {
  return value;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed the function name.
Also, I think I could have simplified the function content without using let 👍

Copy link
Member

Choose a reason for hiding this comment

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

Awesome, I like how these small changes improve readability 😊

@t-hamano t-hamano force-pushed the color-palette/custom-color-css-variable branch from 8302689 to a07c06f Compare January 18, 2023 04:56
@t-hamano
Copy link
Contributor Author

t-hamano commented Jan 18, 2023

Can we also fix the other issue (#41459) in this PR or are you planning to do it separately?

Yes, I would like to try to see if #41459, #44904, can also be solved separately using this function or ref.

Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

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

🚢

@ciampo
Copy link
Contributor

ciampo commented Jan 25, 2023

Thank you @t-hamano for working on this 🙏 it looks like a little improvement but it fixes a really annoying bug!

@juanmaguitar juanmaguitar added the [Type] Bug An existing feature does not function as intended label Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ColorPalette: Color picker doesn't work correctly with variable colors
4 participants