Add tabs for gradient and color#19133
Conversation
83552cb to
7b3d49b
Compare
mcsf
left a comment
There was a problem hiding this comment.
I left some notes for the present and for the future, but nothing blocking!
packages/block-editor/src/components/colors-gradients/control.js
Outdated
Show resolved
Hide resolved
| ); | ||
| } | ||
|
|
||
| function ColorGradientControlSelect( props ) { |
There was a problem hiding this comment.
Observation: some of these component names aren't super clear. It's not a big deal, since this is all private to this file. Some of these are also tied to implementation details (e.g. one of the components reads from useSelect).
Maybe something to keep in mind and polish later if the problem domain is simplifiable.
There was a problem hiding this comment.
Observation: some of these component names aren't super clear. It's not a big deal, since this is all private to this file. Some of these are also tied to implementation details (e.g. one of the components reads from useSelect).
Maybe something to keep in mind and polish later if the problem domain is simplifiable.
That's true; the names are not very clean. These components are tied to implementation details (e.g., useSelect) because they exist to overcome a hooks limitation: hooks should un-unconditionally be called in the same order. In this case, in some situations, we don't need to call useSelect because the props are already available. To overcome the hooks limitation, we have a component that uses useSelect and other that does not, depending on the situation, we use a component or the other. I will try to research/test better solutions to this problem after this PR is merged as I think It will become common soon.
| <Button | ||
| isLarge | ||
| isPrimary={ currentTab === 'color' } | ||
| isSecondary={ currentTab !== 'color' } |
There was a problem hiding this comment.
This is fun. :)
Later, if this PR is a good direction, we should turn this tab group into a proper component.
| onColorChange( newColor ); | ||
| onGradientChange(); | ||
| } : | ||
| onColorChange |
There was a problem hiding this comment.
What is the implication of calling or not calling onColorChange? Could we make it so that it doesn't matter? I suppose the call to onGradientChange with no arguments is a way to unset values, but why can't we just:
<ColorPalette onChange={ ( newColor ) => onColorChange( newColor ); onGradientChange(); } …There was a problem hiding this comment.
We don't have a guarantee that onGradientChange exists, this component may be used just for color picking without gradients.
With the logic, I used we also avoid generating a new function on each render for some cases.
There was a problem hiding this comment.
We don't have a guarantee that onGradientChange exists, this component may be used just for color picking without gradients.
Soon: onGradientsChange?() :)
With the logic, I used we also avoid generating a new function on each render for some cases.
For sure, but a nice thing with hooks such as useCallback is how they allow you to avoid special logic and rely on built-in memoisation.
| { currentTab === 'gradient' && ( | ||
| <GradientPicker | ||
| value={ gradientValue } | ||
| onChange={ canChooseAGradient ? |
There was a problem hiding this comment.
When would a user be picking a gradient if they don't have canChooseAGradient? Or (judging by the symmetry with the previous tab) was this meant to be canChooseAColor?
| const PanelColorGradientSettingsSelect = ( props ) => { | ||
| const colorGradientSettings = useSelect( | ||
| ( select ) => { | ||
| const settings = select( 'core/block-editor' ).getSettings(); | ||
| return pick( settings, colorsAndGradientKeys ); | ||
| } | ||
| ); | ||
| return <PanelColorGradientSettingsInner { ...{ ...colorGradientSettings, ...props } } />; | ||
| }; | ||
|
|
||
| const PanelColorGradientSettings = ( props ) => { | ||
| const relevantProps = pick( props, colorsAndGradientKeys ); | ||
| if ( isEmpty( relevantProps ) || some( | ||
| pick( props, colorsAndGradientKeys ), | ||
| ( setting ) => ( setting === undefined ) | ||
| ) ) { | ||
| return <PanelColorGradientSettingsSelect { ...props } />; | ||
| } | ||
| return <PanelColorGradientSettingsInner { ...props } />; | ||
| }; |
There was a problem hiding this comment.
Just observing that this is the same mechanics as in control.js. Maybe it speaks to a lack of higher-level tooling in our environment (e.g. isEmpty( foo ) || some( foo, isUndefined ), plus the fact that (presumably?) this if statement is meant to avoid calling useSelect in some cases but not others).
There was a problem hiding this comment.
I was making this logic unnecessarily complex, we can simply use every( colorsAndGradientKeys, ( key ) => ( props.hasOwnProperty( key ) ) ).
Co-Authored-By: Miguel Fonseca <miguelcsf@gmail.com>
d66350e to
18fd031
Compare
18fd031 to
c541a27
Compare
Description
This PR implements tabs for Gradient and Solid colors in the blocks that support gradients (Cover and Button) as suggested by @mtias.
In order to that, this PR introduces two new components:
The components existing ColorControl and PanelColorSettings that are equivalent to the new ones but with juts color support were updated to just use the new components.
Currently, the use colors hook already returns PanelColorSettings rendered so I guess we should update the hook to also support gradients as a next step.
cc: @mcsf as he showed interest in checking this PR.
How has this been tested?
I verified I could correctly use the Gradient UI in cover and button.
I verified existing usages of ColorControl and PanelColorSettings in paragraph, group, and heading still work as expected.