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

Add tabs for gradient and color #19133

Merged
merged 3 commits into from Dec 20, 2019
Merged

Conversation

@jorgefilipecosta
Copy link
Member

jorgefilipecosta commented Dec 13, 2019

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:

  • ColorGradientControl component that is responsible for rendering an input control to pick a color or gradient. If only one of those is supported it renders without tabs.
  • PanelColorGradientSettings component that is responsible for rendering panel where the user can pick multiple colors and/or gradients.

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.

@jorgefilipecosta jorgefilipecosta force-pushed the add/gradient-and-color-tabs branch from 83552cb to 7b3d49b Dec 13, 2019
@jorgefilipecosta jorgefilipecosta requested review from nerrad and ntwb as code owners Dec 13, 2019
@jorgefilipecosta jorgefilipecosta changed the title Add tabs for gradient and color tabs Add tabs for gradient and color Dec 17, 2019
@mcsf
mcsf approved these changes Dec 19, 2019
Copy link
Contributor

mcsf left a comment

I left some notes for the present and for the future, but nothing blocking!

);
}

function ColorGradientControlSelect( props ) {

This comment has been minimized.

Copy link
@mcsf

mcsf Dec 19, 2019

Contributor

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.

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Dec 19, 2019

Author Member

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' }

This comment has been minimized.

Copy link
@mcsf

mcsf Dec 19, 2019

Contributor

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
Comment on lines +114 to +117

This comment has been minimized.

Copy link
@mcsf

mcsf Dec 19, 2019

Contributor

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(); } 

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Dec 19, 2019

Author Member

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.

This comment has been minimized.

Copy link
@mcsf

mcsf Dec 19, 2019

Contributor

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 ?

This comment has been minimized.

Copy link
@mcsf

mcsf Dec 19, 2019

Contributor

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 } />;
};
Comment on lines 131 to 150

This comment has been minimized.

Copy link
@mcsf

mcsf Dec 19, 2019

Contributor

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).

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Dec 20, 2019

Author Member

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>
@jorgefilipecosta jorgefilipecosta force-pushed the add/gradient-and-color-tabs branch from d66350e to 18fd031 Dec 20, 2019
@jorgefilipecosta jorgefilipecosta force-pushed the add/gradient-and-color-tabs branch from 18fd031 to c541a27 Dec 20, 2019
@jorgefilipecosta jorgefilipecosta merged commit a97ef27 into master Dec 20, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@jorgefilipecosta jorgefilipecosta deleted the add/gradient-and-color-tabs branch Dec 20, 2019
@youknowriad youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020
@retrofox retrofox mentioned this pull request Jan 8, 2020
1 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.