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.

Sign up for GitHub

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

Try tabs instead of segmented control for switching between solid/gradient in color panels #41937

Merged
merged 12 commits into from
Jul 11, 2022
Merged

Try tabs instead of segmented control for switching between solid/gradient in color panels #41937

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
33784ae
Try tabs in color panels
jameskoster Jun 24, 2022
504ec42
Merge branch 'trunk' into try/tabs-in-color-panels
jameskoster Jul 5, 2022
7859cef
Optimise
jameskoster Jul 5, 2022
8d1a7ff
Fix TabPanel usage
mirka Jul 8, 2022
733ae8b
Fix styles
mirka Jul 8, 2022
4718182
Remove copypasta on screen-link-color
mirka Jul 8, 2022
a48c69b
Fix usage in screen-color-palette
mirka Jul 8, 2022
1b7a34a
Fix styles in screen-background-color
mirka Jul 8, 2022
461c0f9
Remove dead CSS
mirka Jul 8, 2022
7b566da
Update failing test
mirka Jul 8, 2022
587692c
Update test queries
mirka Jul 8, 2022
b77b4df
Remove border beneath tabs, and negative margins in popover
jameskoster Jul 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
142 changes: 77 additions & 65 deletions packages/block-editor/src/components/colors-gradients/control.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,13 @@ import { every, isEmpty } from 'lodash';
/**
* WordPress dependencies
*/
import { useState } from '@wordpress/element';
import {
BaseControl,
__experimentalVStack as VStack,
__experimentalToggleGroupControl as ToggleGroupControl,
__experimentalToggleGroupControlOption as ToggleGroupControlOption,
TabPanel,
ColorPalette,
GradientPicker,
} from '@wordpress/components';
import { __ } from '@wordpress/i18n';

/**
* Internal dependencies
Expand All @@ -30,6 +27,19 @@ const colorsAndGradientKeys = [
'disableCustomGradients',
];

const TAB_COLOR = {
name: 'color',
title: 'Solid color',
value: 'color',
};
const TAB_GRADIENT = {
name: 'gradient',
title: 'Gradient',
value: 'gradient',
};

const TABS_SETTINGS = [ TAB_COLOR, TAB_GRADIENT ];

function ColorGradientControlInner( {
colors,
gradients,
Expand All @@ -52,13 +62,57 @@ function ColorGradientControlInner( {
const canChooseAGradient =
onGradientChange &&
( ! isEmpty( gradients ) || ! disableCustomGradients );
const [ currentTab, setCurrentTab ] = useState(
gradientValue ? 'gradient' : !! canChooseAColor && 'color'
);

if ( ! canChooseAColor && ! canChooseAGradient ) {
return null;
}

const tabPanels = {
[ TAB_COLOR.value ]: (
<ColorPalette
value={ colorValue }
onChange={
canChooseAGradient
? ( newColor ) => {
onColorChange( newColor );
onGradientChange();
}
: onColorChange
}
{ ...{ colors, disableCustomColors } }
__experimentalHasMultipleOrigins={
__experimentalHasMultipleOrigins
}
__experimentalIsRenderedInSidebar={
__experimentalIsRenderedInSidebar
}
clearable={ clearable }
enableAlpha={ enableAlpha }
/>
),
[ TAB_GRADIENT.value ]: (
<GradientPicker
value={ gradientValue }
onChange={
canChooseAColor
? ( newGradient ) => {
onGradientChange( newGradient );
onColorChange();
}
: onGradientChange
}
{ ...{ gradients, disableCustomGradients } }
__experimentalHasMultipleOrigins={
__experimentalHasMultipleOrigins
}
__experimentalIsRenderedInSidebar={
__experimentalIsRenderedInSidebar
}
clearable={ clearable }
/>
),
};

return (
<BaseControl
className={ classnames(
Expand All @@ -78,66 +132,24 @@ function ColorGradientControlInner( {
</legend>
) }
{ canChooseAColor && canChooseAGradient && (
<ToggleGroupControl
value={ currentTab }
onChange={ setCurrentTab }
label={ __( 'Select color type' ) }
hideLabelFromVision
isBlock
>
<ToggleGroupControlOption
value="color"
label={ __( 'Solid' ) }
/>
<ToggleGroupControlOption
value="gradient"
label={ __( 'Gradient' ) }
/>
</ToggleGroupControl>
) }
{ ( currentTab === 'color' || ! canChooseAGradient ) && (
<ColorPalette
value={ colorValue }
onChange={
canChooseAGradient
? ( newColor ) => {
onColorChange( newColor );
onGradientChange();
}
: onColorChange
<TabPanel
className="block-editor-color-gradient-control__tabs"
tabs={ TABS_SETTINGS }
initialTabName={
gradientValue
? TAB_GRADIENT.value
: !! canChooseAColor && TAB_COLOR.value
}
{ ...{ colors, disableCustomColors } }
__experimentalHasMultipleOrigins={
__experimentalHasMultipleOrigins
}
__experimentalIsRenderedInSidebar={
__experimentalIsRenderedInSidebar
}
clearable={ clearable }
enableAlpha={ enableAlpha }
/>
) }
{ ( currentTab === 'gradient' || ! canChooseAColor ) && (
<GradientPicker
value={ gradientValue }
onChange={
canChooseAColor
? ( newGradient ) => {
onGradientChange( newGradient );
onColorChange();
}
: onGradientChange
}
{ ...{ gradients, disableCustomGradients } }
__experimentalHasMultipleOrigins={
__experimentalHasMultipleOrigins
}
__experimentalIsRenderedInSidebar={
__experimentalIsRenderedInSidebar
}
clearable={ clearable }
/>
>
{ ( tab ) => (
<div className="block-editor-color-gradient-control__tab-panel">
{ tabPanels[ tab.value ] }
</div>
) }
</TabPanel>
) }
{ ! canChooseAGradient && tabPanels[ TAB_COLOR.value ] }
{ ! canChooseAColor && tabPanels[ TAB_GRADIENT.value ] }
</VStack>
</fieldset>
</BaseControl>
Expand Down
14 changes: 14 additions & 0 deletions packages/block-editor/src/components/colors-gradients/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,16 @@

.block-editor-panel-color-gradient-settings__dropdown-content .components-popover__content {
width: $sidebar-width;

.block-editor-color-gradient-control__tabs {
margin-top: - $grid-unit-10;
margin-left: - $grid-unit-10;
margin-right: - $grid-unit-10;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if there's a better way to make sure that the Tabs fill the whole width of the sidebar — what about removing the padding from .edit-site-screen-background-color__control and adding it back only to the tab panel's contents?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose avoiding negative values has some merit, but both implementations would still be exposed to the same risk, no? Just trying to fully understand the implications :)

Copy link
Contributor

@ciampo ciampo Jul 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

In general, there isn't anything wrong with negative margins. In this case, it's more about introducing custom style overrides of the TabPanel component (that implicitly relies on the fact that the popover's content has a padding of $grid-unit-10 px).

Specifically to this case, please ignore my initial suggestion re. removing the padding from — I mixed up this instance (which is rendered in a popover) with the one below (rendered in the sidebar).

It looks like the popover's contents have an 8px margin added directly by the Dropdown component (see the code here) (which I'm definitely not a fan of!)

Screenshot 2022-07-05 at 21 13 55

In this specific situation, I'm not sure if there's a clean way to avoid the style override 🤔

@mirka , any ideas?

Copy link
Member

@mirka mirka Jul 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

One solution that comes to mind is how the Card component does it. It has a designated <CardMedia> component that you put inside <Card> when you want to break out of the padding. We could provide a similar subcomponent for Dropdown. ("Media" is not a very discoverable name for this, so ideally we'll give it a more literal name like <DropdownContentFullWidth> or something.)

I'm fine with the hacky style override as a temporary thing for this PR to land, and we can make a DropdownContentFullWidth in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

Looking at the mockups in #38277 (cf. some of the implementations in Global Styles ▸ Colors), I'm wondering whether we've thought through the decision to make the tablist break out of the parent padding or not. There seems to be a lot of hierarchical contexts where it would be better not to be full-width. I wanted to check on this point because it does add a good amount of complexity and fragmentation risk if we don't have a clear style guide.

It's totally fine if some top-level tablists are full-width (e.g. Blocks/Patterns, Template/Block, Post/Block). But for things that aren't really top-level, or for components that are reused in multiple contexts such as this ColorGradientControl, things will be a lot simpler if we don't go full-width. Again, your call, but just a consideration from the maintenance/consistency standpoint 🙂 (cc @javierarce)


.block-editor-color-gradient-control__tab-panel {
padding: $grid-unit-10;
}
}


Expand Down Expand Up @@ -100,3 +110,7 @@
.block-editor-panel-color-gradient-settings__dropdown {
width: 100%;
}

.block-editor-color-gradient-control__tab-panel {
border-top: 1px solid $gray-300;
}
Copy link
Member

@mirka mirka Jul 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

FWIW, after working on this PR, I'm now a little apprehensive about adding in this ad hoc border. The Global Styles ▸ Colors menu already contains several tabbed color panels that have different implementations, easily leading to style fragmentation. See also #41976 which seems to have landed recently. Your call, but it might be better to leave the border off for now and address the border issue in a separate PR.

52 changes: 25 additions & 27 deletions packages/edit-site/src/components/global-styles/screen-color-palette.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,7 @@
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import {
__experimentalToggleGroupControl as ToggleGroupControl,
__experimentalToggleGroupControlOption as ToggleGroupControlOption,
} from '@wordpress/components';
import { useState } from '@wordpress/element';
import { TabPanel } from '@wordpress/components';

/**
* Internal dependencies
Expand All @@ -16,8 +12,6 @@ import GradientPalettePanel from './gradients-palette-panel';
import ScreenHeader from './header';

function ScreenColorPalette( { name } ) {
const [ currentTab, setCurrentTab ] = useState( 'solid' );

return (
<>
<ScreenHeader
Expand All @@ -26,27 +20,31 @@ function ScreenColorPalette( { name } ) {
'Palettes are used to provide default color options for blocks and various design tools. Here you can edit the colors with their labels.'
) }
/>
<ToggleGroupControl
className="edit-site-screen-color-palette-toggle"
value={ currentTab }
onChange={ setCurrentTab }
label={ __( 'Select palette type' ) }
hideLabelFromVision
isBlock
<TabPanel
tabs={ [
{
name: 'solid',
title: 'Solid color',
value: 'solid',
},
{
name: 'gradient',
title: 'Gradient',
value: 'gradient',
},
] }
>
<ToggleGroupControlOption
value="solid"
label={ __( 'Solid' ) }
/>
<ToggleGroupControlOption
value="gradient"
label={ __( 'Gradient' ) }
/>
</ToggleGroupControl>
{ currentTab === 'solid' && <ColorPalettePanel name={ name } /> }
{ currentTab === 'gradient' && (
<GradientPalettePanel name={ name } />
) }
{ ( tab ) => (
<>
{ tab.value === 'solid' && (
<ColorPalettePanel name={ name } />
) }
{ tab.value === 'gradient' && (
<GradientPalettePanel name={ name } />
) }
</>
) }
</TabPanel>
</>
);
}
Expand Down
2 changes: 1 addition & 1 deletion packages/edit-site/src/components/global-styles/screen-link-color.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ function ScreenLinkColor( { name } ) {
) }
/>

<TabPanel className="my-tab-panel" tabs={ tabs }>
Copy link
Member

Choose a reason for hiding this comment

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

🧹 Removed some copypasta here

<TabPanel tabs={ tabs }>
{ ( tab ) => {
const pseudoSelectorConfig =
pseudoStates[ tab.name ] ?? null;
Expand Down
9 changes: 7 additions & 2 deletions packages/edit-site/src/components/global-styles/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,11 +77,16 @@

.edit-site-screen-text-color__control,
.edit-site-screen-link-color__control,
.edit-site-screen-button-color__control,
.edit-site-screen-background-color__control {
.edit-site-screen-button-color__control {
padding: $grid-unit-20;
}

.edit-site-screen-background-color__control {
.block-editor-color-gradient-control__tab-panel {
padding: $grid-unit-20;
}
}

.edit-site-global-styles-variations_item {
box-sizing: border-box;

Expand Down