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

Reverts ColorPalette disableAlpha #18175 #18220

Merged
merged 1 commit into from
Nov 11, 2019

Conversation

mkaz
Copy link
Member

@mkaz mkaz commented Oct 31, 2019

Description

Reverts the change made in #18175 introducing the disableAlpha property to pass forward.

@youknowriad raised some good issues since the ColorPalette returns an hex code, that does not accept the alpha parameter it makes the additional feature unusable, or at least setting the channel does not get returned with the parameters.

The solution is likely to switch to returning rgba syntax if disableAlpha is false, reverting that change so we can address.

How has this been tested?

Previous state prior to #18175 being enabled.

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

Hi @mkaz, I checked the reason why this was being disabled:
onChangeComplete={ ( color ) => onChange( color.hex )

It seems like we could change that code and if an alpha component is set instead of passing a text color one may pass a color in the format "rgba( ... )".
I think the main question now is do we want to continue the update and support an alpha channel?

@mkaz
Copy link
Member Author

mkaz commented Oct 31, 2019

@jorgefilipecosta heh, thanks - I rushed to put the revert together and I was going to look at the code after. Fixing up the code looks like it might be easier than the revert.

Any reason we wouldn't want to support it? The underlying React component supports it, so seems fair to give that flexibility to someone using this component.

@youknowriad
Copy link
Contributor

rgba when this prop is enabled is just one of the different options, maybe we can revert in order to put some thoughts about the best way forward?

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

I'm going to land this to avoid including in the next Gutenberg release.

@youknowriad youknowriad merged commit bf5c163 into master Nov 11, 2019
@youknowriad youknowriad deleted the revert/18175-color-palette branch November 11, 2019 10:03
@youknowriad youknowriad modified the milestones: Future, Gutenberg 6.9 Nov 11, 2019
@jorgefilipecosta
Copy link
Member

Any reason we wouldn't want to support it? The underlying React component supports it, so seems fair to give that flexibility to someone using this component.

I don't think we have a reason for not including it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants