Allow "disableAlpha" for color Picker component to be optional (replaces #5835) #7151

diegoliv · 2018-06-05T15:54:40Z

Description

Currently, the <ColorPalette /> component enforces the alpha channel from react-color to be disabled. Like described on #5811, there are benefits of giving developers the option to enable the alpha channel inside the color picker.

Previously #5835
Fixes #4726

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 disableAlpha parameter for the <ColorPallete /> component. It is set to false by 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 the hex value for the color. If opacity has a different level, then we return the rgba() value for the color.

…nel inside color picker

diegoliv · 2018-06-05T15:55:33Z

@danielbachhuber here is an updated PR that replaces #5835. Take a look and let me know if is there anything to be tweaked.

danielbachhuber

👍 Looks good to me! Flagging a second review.

tofumatt

Largely seems fine but there are a few tweaks I think you could make to tidy up this code.

tofumatt · 2018-06-06T04:24:18Z

components/color-palette/index.js

@@ -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 } ) {


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.

@tofumatt done!

tofumatt · 2018-06-06T04:29:03Z

components/color-palette/index.js

+									colorString = color.hex;
+								} else {
+									const { r, g, b, a } = color.rgb;
+									colorString = 'rgba(' + [ r, g, b, a ].join( ',' ) + ')';


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 Array.join is being used at all here, it just seems to make it more complex.

@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.

tofumatt · 2018-06-06T04:30:57Z

components/color-palette/index.js

+									const { r, g, b, a } = color.rgb;
+									colorString = 'rgba(' + [ r, g, b, a ].join( ',' ) + ')';
+								}
+								onChange( colorString );


Seems there's a code path here where colorString is undefined, should we test for its truthiness before running onChange?

Seems there's a code path here where colorString is undefined

What's the code path?

Oh wait, there isn't, but because it's defined as undefined above it sort of read like maybe there could be.

I actually see why it's this way, never mind me, all good!

…gutenberg into update/disableAlpha_v2

danielbachhuber · 2018-06-14T23:31:03Z

Holding on merge per #7246. Sorry about that, @diegoliv — I appreciate the work you've put into this.

gziolo · 2018-06-21T07:57:23Z

Moving this to 3.2 Milestone as it seems to be blocked with #7246. Do I read it properly @danielbachhuber? Feel free to remove the label I'm about to add.

danielbachhuber · 2018-06-21T12:48:20Z

Yes, we're currently blocked by #7246

TheoL · 2018-11-05T18:58:32Z

#7246 has been fixed. Can you now proceed with this?

tofumatt · 2018-11-05T19:01:13Z

I'm fine with that but we probably won't look to merge it until after 5.0 ships as it's a reasonable UI change and we are trying to freeze the UI–at least as much as possible. 😄

TheoL · 2018-11-05T19:17:49Z

disableAlpha will still default to true so from a UI perspective it shouldn't be an issue - existing code will continue to work in the same way.

I need this for a plugin I am creating which I was hoping to release when 5.0 is officially out.

tofumatt · 2018-11-05T19:26:35Z

Ah right; sorry, it's been awhile since I looked at the issue. Makes sense to me!

gziolo · 2018-11-19T09:33:17Z

It needs to be rebased with the latest changes from master branch.

earnjam · 2018-12-21T01:06:42Z

I just ran into this issue. It would be nice to tidy up the branch and try to get this shipped. @diegoliv are you still working on this?

diegoliv · 2018-12-21T02:25:17Z

@earnjam I stopped working on this since the team started working on #7246 and this PR was put on hold. I can pull the latest version and fix the PR.

gziolo

@diegoliv, that would be great to get it up to date 👍

gziolo · 2019-04-24T08:52:10Z

This Pull Request wasn't updated for quite some time. Let's close it to let other contributors work on it. @diegoliv you can still refresh your work and ask for another round of review. It looks like this proposal is very close to be ready. Thank you for your contribution so far!

updated master branch and added disableAlpha prop to allow alpha chan…

de4bda1

…nel inside color picker

Revert changes to package-lock.json

625f938

danielbachhuber requested a review from a team June 5, 2018 23:26

danielbachhuber approved these changes Jun 5, 2018

View reviewed changes

danielbachhuber mentioned this pull request Jun 5, 2018

Allow "disableAlpha" for color Picker component to be optional #5835

Closed

danielbachhuber added this to the 3.1 milestone Jun 5, 2018

tofumatt reviewed Jun 6, 2018

View reviewed changes

diegoliv added 4 commits June 8, 2018 10:59

sorted component props alphabetically

b4c1a09

replaced Array.join with a template string

799fa3e

small tweak to template string to match snapshot

ea87187

Merge branch 'update/disableAlpha_v2' of https://github.com/diegoliv/…

359c18c

…gutenberg into update/disableAlpha_v2

afercia mentioned this pull request Jun 10, 2018

The ChromePicker (Custom color picker) is not accessible #7246

Closed

gziolo added the [Status] Blocked Used to indicate that a current effort isn't able to move forward label Jun 21, 2018

gziolo modified the milestones: 3.1, 3.2 Jun 21, 2018

danielbachhuber removed this from the 3.2 milestone Jun 21, 2018

gziolo requested changes Jan 25, 2019

View reviewed changes

gziolo added [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. and removed [Status] Blocked Used to indicate that a current effort isn't able to move forward labels Feb 1, 2019

gziolo mentioned this pull request Apr 24, 2019

ColorPalette: enable alpha channel #4726

Closed

gziolo closed this Apr 24, 2019

gziolo added [Feature] UI Components Impacts or related to the UI component system [Type] Enhancement A suggestion for improvement. labels Apr 24, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow "disableAlpha" for color Picker component to be optional (replaces #5835) #7151

Allow "disableAlpha" for color Picker component to be optional (replaces #5835) #7151

diegoliv commented Jun 5, 2018 •

edited by gziolo

Loading

diegoliv commented Jun 5, 2018

danielbachhuber left a comment

tofumatt left a comment

tofumatt Jun 6, 2018

diegoliv Jun 8, 2018

tofumatt Jun 6, 2018

diegoliv Jun 8, 2018

tofumatt Jun 6, 2018

danielbachhuber Jun 6, 2018

tofumatt Jun 6, 2018

danielbachhuber commented Jun 14, 2018

gziolo commented Jun 21, 2018

danielbachhuber commented Jun 21, 2018

TheoL commented Nov 5, 2018

tofumatt commented Nov 5, 2018

TheoL commented Nov 5, 2018 •

edited

Loading

tofumatt commented Nov 5, 2018 via email

gziolo commented Nov 19, 2018

earnjam commented Dec 21, 2018

diegoliv commented Dec 21, 2018

gziolo left a comment

gziolo commented Apr 24, 2019

Allow "disableAlpha" for color Picker component to be optional (replaces #5835) #7151

Allow "disableAlpha" for color Picker component to be optional (replaces #5835) #7151

Conversation

diegoliv commented Jun 5, 2018 • edited by gziolo Loading

Description

Tested With

Types of changes

diegoliv commented Jun 5, 2018

danielbachhuber left a comment

Choose a reason for hiding this comment

tofumatt left a comment

Choose a reason for hiding this comment

tofumatt Jun 6, 2018

Choose a reason for hiding this comment

diegoliv Jun 8, 2018

Choose a reason for hiding this comment

tofumatt Jun 6, 2018

Choose a reason for hiding this comment

diegoliv Jun 8, 2018

Choose a reason for hiding this comment

tofumatt Jun 6, 2018

Choose a reason for hiding this comment

danielbachhuber Jun 6, 2018

Choose a reason for hiding this comment

tofumatt Jun 6, 2018

Choose a reason for hiding this comment

danielbachhuber commented Jun 14, 2018

gziolo commented Jun 21, 2018

danielbachhuber commented Jun 21, 2018

TheoL commented Nov 5, 2018

tofumatt commented Nov 5, 2018

TheoL commented Nov 5, 2018 • edited Loading

tofumatt commented Nov 5, 2018 via email

gziolo commented Nov 19, 2018

earnjam commented Dec 21, 2018

diegoliv commented Dec 21, 2018

gziolo left a comment

Choose a reason for hiding this comment

gziolo commented Apr 24, 2019

diegoliv commented Jun 5, 2018 •

edited by gziolo

Loading

TheoL commented Nov 5, 2018 •

edited

Loading