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

Update ColorPalette component to accept disableAlpha parameter #18175

Merged
merged 2 commits into from
Oct 30, 2019
Merged

Update ColorPalette component to accept disableAlpha parameter #18175

merged 2 commits into from
Oct 30, 2019

Conversation

mkaz
Copy link
Member

@mkaz mkaz commented Oct 29, 2019

Description

Adds the disableAlpha parameter to ColorPalette component which is passed forward to the ColorPicker to show. Previously it was set to true by default.

Fixes #4726

How has this been tested?

Run npm run design-system:dev to view component in Storybook.
Click custom color and confirm the Alpha channel shows up.

Screenshots

Screenshot from 2019-10-29 15-24-18

Types of changes

Update ColorPalette component previously hard-coded to false when using the ColorPicker, this allows the parameter to be set by user.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

Adds the disableAlpha parameter to ColorPalette component which is
passed forward to the ColorPicker to show. Previously it was set to true
by default. Fixes #4726
@mkaz mkaz added [Package] Components /packages/components Storybook Storybook and its stories for components labels Oct 29, 2019
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Nice fix, thank you. Let's make sure this change is included in the docs before merging.

@mkaz mkaz merged commit 96f7d34 into WordPress:master Oct 30, 2019
@mkaz mkaz deleted the update/4726-color-palette branch October 30, 2019 17:29
@@ -55,7 +56,7 @@ export default function ColorPalette( {
<ColorPicker
color={ value }
onChangeComplete={ ( color ) => onChange( color.hex ) }
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I know hex colors format don't include "alpha" information in it. This make me think this PR is not complete yet as there's no way for the users of this component to get the value of the selected alpha.

Should we switch to rgba if disableAlpha is false? should we do it every time? do we need a format prop? should we pass the whole color object. These are all options.

Copy link
Member

Choose a reason for hiding this comment

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

Should this PR be reverted?

Copy link
Member Author

Choose a reason for hiding this comment

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

ah... that might explain why it was disabled in the first place.
I think we should revert and then we can add to address other issues.
I prepped this PR for the revert: #18220

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components Storybook Storybook and its stories for components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ColorPalette: enable alpha channel
3 participants