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

Adds ColorControl component #19288

Open
wants to merge 4 commits into
base: master
from

Conversation

@ryanwelcher
Copy link
Contributor

ryanwelcher commented Dec 21, 2019

Description

Adds ColorControl component. Closes #14378

How has this been tested?

Tests still to be implemented. I have tested this locally on master using the example provided in the README.

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.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .
Copy link
Member

jorgefilipecosta left a comment

Hi @ryanwelcher, thank you for this PR! Currently, we are exporting PanelColorSettings (packages/block-editor/src/components/panel-color-settings/index.js). Would it be possible to provide some details of why PanelColorSettings did not accomplished your needs?

@chrisvanpatten

This comment has been minimized.

Copy link
Member

chrisvanpatten commented Dec 24, 2019

@jorgefilipecosta PanelColorSettings provides a palette, right? A ColorControl would be more useful for cases where you aren’t providing a palette.

We’re also using ColorPicker directly in a sidebar (we don’t support defined palettes, it’s all freeform for our editorial teams) so I think this could be very valuable.

Copy link
Member

chrisvanpatten left a comment

Definitely support this; we started wrapping ColorPicker in a BaseControl ourselves recently so I can see a use for this.

@ryanwelcher

This comment has been minimized.

Copy link
Contributor Author

ryanwelcher commented Jan 2, 2020

Hi @ryanwelcher, thank you for this PR! Currently, we are exporting PanelColorSettings (packages/block-editor/src/components/panel-color-settings/index.js). Would it be possible to provide some details of why PanelColorSettings did not accomplished your needs?

@jorgefilipecosta I wanted more flexibility for color choice/options.

@ryanwelcher ryanwelcher requested a review from chrisvanpatten Jan 3, 2020
@ryanwelcher

This comment has been minimized.

Copy link
Contributor Author

ryanwelcher commented Jan 7, 2020

@chrisvanpatten I've addressed your feedback. Is there anything else I can do to move this forward? cc @jorgefilipecosta

@chrisvanpatten

This comment has been minimized.

Copy link
Member

chrisvanpatten commented Jan 7, 2020

Sorry for the delay @ryanwelcher — I'm pretty satisfied with this from a code perspective but I think we'd probably need signoff from a lead on including this component.

I agree on offering it, and I think it makes a ton of sense; I know I'd use it.

@chrisvanpatten

This comment has been minimized.

Copy link
Member

chrisvanpatten commented Jan 7, 2020

It might make sense to raise this during tomorrow's #core-editor chat; I think that'd be an appropriate venue to bring it up and ask for reviews.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Jan 9, 2020

I'm pretty sure I saw a mockup revisiting the color selectors to use a nice color control (related to #18667) cc @jasmussen if you have any idea where to find this.

I wonder if this could be the opportunity to start leveraging those in some blocks.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Jan 9, 2020

Hi! Not sure if this is helpful, or even necessary for this PR, but thought I'd share a mockups that are being explored as part of #18667:

Screenshot 2020-01-09 at 10 40 22

In this interface, the color picker would hold color swatches, like Figma:

Screenshot 2020-01-09 at 10 42 00

But this is an aspect to explore a bit further as we get to it!

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Jan 9, 2020

It would be cool to add a storybook story to this PR to be able to test the component properly.

@ryanwelcher

This comment has been minimized.

Copy link
Contributor Author

ryanwelcher commented Jan 14, 2020

@youknowriad I have added stories for the new component.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Jan 14, 2020

Should we try to implement the proposed UI #19288 (comment) ?

@ryanwelcher ryanwelcher force-pushed the ryanwelcher:add/color-control-component branch from 402ebe9 to 885f7d9 Jan 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.