-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Allow "disableAlpha" for color Picker component to be optional (replaces #5835) #7151
Changes from 2 commits
de4bda1
625f938
b4c1a09
799fa3e
ea87187
359c18c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,7 +18,7 @@ import { __, sprintf } from '@wordpress/i18n'; | |
import './style.scss'; | ||
import Button from '../button'; | ||
|
||
export default function ColorPalette( { colors, disableCustomColors = false, value, onChange } ) { | ||
export default function ColorPalette( { colors, disableCustomColors = false, value, onChange, disableAlpha = true } ) { | ||
function applyOrUnset( color ) { | ||
return () => onChange( value === color ? undefined : color ); | ||
} | ||
|
@@ -65,8 +65,17 @@ export default function ColorPalette( { colors, disableCustomColors = false, val | |
renderContent={ () => ( | ||
<ChromePicker | ||
color={ value } | ||
onChangeComplete={ ( color ) => onChange( color.hex ) } | ||
disableAlpha | ||
onChangeComplete={ ( color ) => { | ||
let colorString; | ||
if ( typeof color.rgb === 'undefined' || color.rgb.a === 1 ) { | ||
colorString = color.hex; | ||
} else { | ||
const { r, g, b, a } = color.rgb; | ||
colorString = 'rgba(' + [ r, g, b, a ].join( ',' ) + ')'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could use template strings here and omit the join; it'd probably be a bit more readable: colorString = `rgba(${ r }, ${ g }, ${ b }, ${ a })`; I'm not sure why the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tofumatt yeah, I guess this is me still not 100% used to use ES6 features. But I totally agree with you. Just switched the variable to template strings. |
||
} | ||
onChange( colorString ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems there's a code path here where There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
What's the code path? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh wait, there isn't, but because it's defined as I actually see why it's this way, never mind me, all good! |
||
} } | ||
disableAlpha={ disableAlpha } | ||
/> | ||
) } | ||
/> | ||
|
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.
A nitpick but it might be nice to sort these props alphabetically. The current ordering makes it harder to sort through arguments and makes this last one feel especially tacked-on.
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.
@tofumatt done!