-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
: Display checkered preview background when value
is transparent
#42232
ColorPalette
: Display checkered preview background when value
is transparent
#42232
Conversation
@WordPress/gutenberg-design for thoughts here :) |
@jameskoster What's the best way to detect the alpha channel? |
Sorry, I'm not sure 🙈 |
@jameskoster It turned out to be pretty simple! Updated in bcfb85a |
background
when value
is 'transparent'background
when value
is transparent
background
when value
is transparentbackground
when value
is transparent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. @jameskoster we should probably update the transparent circle in the palette to match the newer transparent grid design
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix! If you could add a quick changelog entry before merging, that would be great.
In a follow-up, we may want to look into tweaking the background implementation so the checkered background can be seen through a semi-transparent color. This is already the case with the circular swatch:
export const showTransparentBackground = ( currentValue ) => { | ||
return colord( currentValue ).alpha() === 0; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add handling (and a test case) for an undefined
color value? It will be undefined when the "Clear" button is clicked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
background
when value
is transparentColorPalette
: Display checkered preview background when value
is transparent
Co-authored-by: Lena Morita <[email protected]>
@mirka You're welcome 😄 Did with 23f9b51
I actually looked into this. I think it will involve a bit of CSS retooling, though. I didn't feel comfortable contributing the change because I felt like it might delay the review process (and solving 'transparent' is my immediate need). |
test( 'should return undefined for transparent colors', () => { | ||
expect( undefined ).toBe( undefined ); | ||
} ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah sorry this was unclear, I think the behavior we want is for it to return true:
test( 'should return undefined for transparent colors', () => { | |
expect( undefined ).toBe( undefined ); | |
} ); | |
test( 'should return true for undefined color values', () => { | |
expect( undefined ).toBe( true ); | |
} ); |
The fact that it "works" as expected with the current background
implementation is only coincidental. It will be misleading to return a falsy value from showTransparentBackground( undefined )
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LOL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're good to go then! 🚀
I actually looked into this. I think it will involve a bit of CSS retooling, though. I didn't feel comfortable contributing the change because I felt like it might delay the review process (and solving 'transparent' is my immediate need).
Agreed, this is the right call 👍 Split into a separate issue at #42715.
Awesome, thanks! |
What?
Avoids styling
background
when the providedvalue
is transparent (eithertransparent
, or a RGBA/HEX value with transparent alpha channel).After
Before
Why?
Fixes #42230