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

Enable color palettes with custom color alpha support #30493

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

niklasp
Copy link
Contributor

@niklasp niklasp commented Apr 3, 2021

Description

<ColorPicker> component already had alpha support. Added a disableAlpha prop to the <ColorPalette> that passes the prop to the <ColorPicker>. Fix #27960 Fix #13045

How has this been tested?

Tested with a custom Block that has a <ColorPalette> in its inspector controls and disableAlpha={ false }. Also tested core components that have <ColorPalette> still work.

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@Mamaduka
Copy link
Member

Mamaduka commented Apr 6, 2021

The change looks good to me.

It looks like checks failed because of the Docker registry, so I restarted them.

Update: I just came across this comment from a previous PR that tried to add this feature.

Based on it, this PR might not be complete.

@Mamaduka Mamaduka added [Package] Components /packages/components [Type] Feature New feature to highlight in changelogs. labels Apr 6, 2021
@niklasp
Copy link
Contributor Author

niklasp commented Apr 6, 2021

I see that there might be the need for discussion. I could oc write Story and change the doc as well. Other than the PR you posted I just decided an rgba color format. But I do not see where this might be a problem?

Also could you forward the issue to someone who could decide that or open the descussion? Since css 4 spec we could also use 8 digit hex codes including alpha information.

@niklasp
Copy link
Contributor Author

niklasp commented Apr 10, 2021

Also to add to the discussion. The colorpicker does the same afaik, return hex values when alpha is disabled and rgba when it is enabled. https://wordpress.github.io/gutenberg/?path=/story/components-colorpicker--alpha-enabled. @youknowriad what is your opinion on this?

@youknowriad
Copy link
Contributor

Also to add to the discussion. The colorpicker does the same afaik, return hex values when alpha is disabled and rgba when it is enabled. https://wordpress.github.io/gutenberg/?path=/story/components-colorpicker--alpha-enabled. @youknowriad what is your opinion on this?

I think that's fine though we might want to document the prop and the behavior properly on the README of the component.

ColorPicker component already had alpha support. Added a `disableAlpha` prop to the `<ColorPalette>` that passes the prop to the ColorPicker. Fix WordPress#27960 WordPress#13045
@niklasp
Copy link
Contributor Author

niklasp commented Apr 13, 2021

@Mamaduka added storybook example and doc. Tests succeed.

@niklasp
Copy link
Contributor Author

niklasp commented Apr 28, 2021

@ajitbohra @chrisvanpatten ?

@niklasp
Copy link
Contributor Author

niklasp commented May 28, 2021

Still waiting for this to be merged, is there a reason why it is still pending reviews? @ajitbohra @chrisvanpatten?

@lukecarbis
Copy link
Contributor

I'm waiting for this to be merged also.

@lukecarbis lukecarbis added the Needs Technical Feedback Needs testing from a developer perspective. label Aug 4, 2021
@skorasaurus skorasaurus added the [Feature] Colors Color management label Mar 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Colors Color management Needs Technical Feedback Needs testing from a developer perspective. [Package] Components /packages/components [Type] Feature New feature to highlight in changelogs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ColorPalette disableAlpha=false not working Alpha support for ColorPalette custom color
5 participants