Allow "disableAlpha" for color Picker component to be optional #5835
Conversation
…update/disableAlpha
|
Related: #5382 |
blocks/color-palette/index.js
Outdated
| @@ -59,9 +59,13 @@ export function ColorPalette( { colors, disableCustomColors = false, value, onCh | |||
| renderContent={ () => ( | |||
| <ChromePicker | |||
| color={ value } | |||
| onChangeComplete={ ( color ) => onChange( color.hex ) } | |||
| // onChangeComplete={ ( color ) => onChange( color.hex ) } | |||
There was a problem hiding this comment.
Commented line should be removed.
blocks/color-palette/index.js
Outdated
| onChangeComplete={ ( color ) => onChange( color.hex ) } | ||
| // onChangeComplete={ ( color ) => onChange( color.hex ) } | ||
| onChangeComplete={ ( color ) => { | ||
| const colorString = color.rgb.a === 1 ? color.hex : sprintf( 'rgba(%s,%s,%s,%s)', color.rgb.r, color.rgb.g, color.rgb.b, color.rgb.a ); |
There was a problem hiding this comment.
Unlike in PHP, sprintf is not a native function, and could have performance implications. While this is not a function we would expect to be called with any frequency, I'd rather we avoided become too comfortable with its use where simple concatenation would suffix.
You could, for example, take advantage of Array#join's default argument being a comma:
let colorString;
if ( color.rgb.a === 1 ) {
colorString = color.hex;
} else {
const { r, g, b, a } = color.rgb;
colorString = `rgba(${ [ r, g, b, a ].join() })`;
}(Or be explicit with .join( ',' ))
|
@aduth thanks for reviewing my PR! Ok, I think that the review request is now addressed. Let me know if it's all good. |
|
@diegoliv Looks like you have a test failing on: Can you address this issue? |
|
@danielbachhuber ok, now it seems that all tests have passed. Let me know what you think! |
|
@diegoliv Looks better. Can you include a test or two for your change? |
|
@danielbachhuber if you're talking about writing some unit tests, I'm sorry, I don't have much experience with that. Can someone help me on this? I can work on something with the right guidance. Now, if you're talking about real tests, I tested the
All of my tests worked as expected. I also recorded a quick video showing the alpha channel in action on my custom block. Link to the video here. |
|
@diegoliv Apologies for the delay! I am talking about unit tests. However, I'm not yet an expert on Gutenberg's test suite, so I'll pull what I can together for you :)
Hope this helps! @aduth Feel free to weigh in with any other details you think relevant. |
|
@danielbachhuber thanks for the help! I don't have any experience with unit tests, so this is a good place to start learning. The problem is that I'm not sure if I'm doing it right, since I'm learning as I go, so I probably need someone with more experience to review my work. I just pushed to this PR two new tests, can you take a look and let me know if I'm on the right path or not? Thanks! |
blocks/color-palette/test/index.js
Outdated
| @@ -45,19 +47,19 @@ describe( 'ColorPalette', () => { | |||
|
|
|||
| test( 'should call onClick with undefined, when the clearButton onClick is triggered', () => { | |||
| const clearButton = wrapper.find( '.button-link.blocks-color-palette__clear' ); | |||
|
|
|||
There was a problem hiding this comment.
These lines with added whitespace should be removed
Your IDE or Editor probably has an option along the lines of "When enabled, will trim trailing whitespace when saving a file."
You can also install an "Editor Config" extension for your IDE/Editor from http://editorconfig.org/#download that will then inherit the Gutenberg settings from https://github.com/WordPress/gutenberg/blob/master/.editorconfig
blocks/color-palette/test/index.js
Outdated
| expect( clearButton ).toHaveLength( 1 ); | ||
|
|
||
There was a problem hiding this comment.
extraneous whitespace should be removed
blocks/color-palette/test/index.js
Outdated
| clearButton.simulate( 'click' ); | ||
|
|
||
There was a problem hiding this comment.
extraneous whitespace should be removed
blocks/color-palette/test/index.js
Outdated
| expect( onChange ).toHaveBeenCalledTimes( 1 ); | ||
| expect( onChange ).toHaveBeenCalledWith( undefined ); | ||
| } ); | ||
|
|
||
There was a problem hiding this comment.
extraneous whitespace should be removed
blocks/color-palette/test/index.js
Outdated
| test( 'should allow disabling custom color picker', () => { | ||
| expect( shallow( <ColorPalette colors={ colors } disableCustomColors={ true } value={ currentColor } onChange={ onChange } /> ) ).toMatchSnapshot(); | ||
| } ); | ||
|
|
||
There was a problem hiding this comment.
extraneous whitespace should be removed
blocks/color-palette/test/index.js
Outdated
| } ); | ||
| } ); | ||
|
|
There was a problem hiding this comment.
extraneous new line here should be removed
|
@ntwb thanks for the heads up on the white spaces, I'm using Visual Studio Code on Windows. Just fixed the white spaces and line breaks. It should be fine now! |
…enberg into update/disableAlpha
|
@diegoliv Sorry for the delay here. Can you resolve the merge conflict and then we'll get this landed? |
|
@danielbachhuber ok, just removed the legacy
Am I doing something wrong? |
|
@danielbachhuber ok, so I forgot to run Here's what I'm seeing: |
| @@ -830,7 +830,27 @@ function gutenberg_common_scripts_and_styles() { | |||
| function gutenberg_enqueue_registered_block_scripts_and_styles() { | |||
| $is_editor = ( 'enqueue_block_editor_assets' === current_action() ); | |||
|
|
|||
| // parse and prepare blocks attributes to be used on the 'assets_callback' call | |||
| if( !$is_editor && is_singular() ) { | |||
There was a problem hiding this comment.
This shouldn't be here....
|
It looks like some unexpected changes snuck into this pull request. #6956 might be related to the unexpected test failures. Can you port your changes over to a new pull request and we can work from there? |
|
@danielbachhuber ok, so I updated my local repo and tried to run Also, running npm run build throws this error: I'm doing this on the |
Not sure, to be honest. Are you running node |
|
@danielbachhuber yep, using 8.0.0. I'll check on Slack if someone can help me on this. |
|
Closing in favor of #7151 |
Description
Currently, the
<ColorPalette />component enforces the alpha channel fromreact-colorto be disabled. Like described on #5811, there are benefits of giving developers the option to enable the alpha channel inside the color picker.Tested With
WordPress 4.9.4
Gutenberg 2.4.0
PHP 7.0.3 / nginx
MySQL 5.5.55
Types of changes
This PR introduces the ability to conditionally set a
disableAlphaparameter for the<ColorPallete />component. It is set tofalseby default so it won't conflict with existing blocks that uses this component. Also, the returned color string depends on how the alpha channel inside the color picker is set. If opacity is 100%, we just return thehexvalue for the color. If opacity has a different level, then we return thergba()value for the color.