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

Refactor the colors UI in the inspector to use the navigator approach #36952

Closed
wants to merge 2 commits into from
Closed

Refactor the colors UI in the inspector to use the navigator approach #36952

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6344651
Refactor the colors UI in the inspector to use the navigator pattern
youknowriad Nov 29, 2021
29c49a8
Try adapting the padding to work in the classic inspector.
jasmussen Dec 1, 2021
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
31 changes: 13 additions & 18 deletions packages/block-editor/src/hooks/color-panel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import { __ } from '@wordpress/i18n';
*/
import PanelColorGradientSettings from '../components/colors-gradients/panel-color-gradient-settings';
import ContrastChecker from '../components/contrast-checker';
import InspectorControls from '../components/inspector-controls';
import { __unstableUseBlockRef as useBlockRef } from '../components/block-list/use-block-props/use-block-refs';

function getComputedStyle( node ) {
Expand All @@ -20,7 +19,6 @@ export default function ColorPanel( {
settings,
clientId,
enableContrastChecking = true,
showTitle = true,
} ) {
const [ detectedBackgroundColor, setDetectedBackgroundColor ] = useState();
const [ detectedColor, setDetectedColor ] = useState();
Expand Down Expand Up @@ -54,21 +52,18 @@ export default function ColorPanel( {
} );

return (
<InspectorControls>
<PanelColorGradientSettings
title={ __( 'Color' ) }
initialOpen={ false }
settings={ settings }
showTitle={ showTitle }
__experimentalHasMultipleOrigins
>
{ enableContrastChecking && (
<ContrastChecker
backgroundColor={ detectedBackgroundColor }
textColor={ detectedColor }
/>
) }
</PanelColorGradientSettings>
</InspectorControls>
<PanelColorGradientSettings
showTitle={ false }
title={ __( 'Color' ) }
settings={ settings }
__experimentalHasMultipleOrigins
>
{ enableContrastChecking && (
<ContrastChecker
backgroundColor={ detectedBackgroundColor }
textColor={ detectedColor }
/>
) }
</PanelColorGradientSettings>
);
}
275 changes: 219 additions & 56 deletions packages/block-editor/src/hooks/color.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,25 @@ import { isObject, setWith, clone } from 'lodash';
*/
import { addFilter } from '@wordpress/hooks';
import { getBlockSupport } from '@wordpress/blocks';
import { __ } from '@wordpress/i18n';
import { __, isRTL } from '@wordpress/i18n';
import { useRef, useEffect, useMemo, Platform } from '@wordpress/element';
import { createHigherOrderComponent } from '@wordpress/compose';
import {
__experimentalNavigatorProvider as NavigatorProvider,
__experimentalNavigatorScreen as NavigatorScreen,
__experimentalUseNavigator as useNavigator,
__experimentalItemGroup as ItemGroup,
__experimentalItem as Item,
__experimentalView as View,
__experimentalHStack as HStack,
__experimentalVStack as VStack,
__experimentalSpacer as Spacer,
__experimentalHeading as Heading,
FlexItem,
ColorIndicator,
PanelBody,
} from '@wordpress/components';
import { Icon, chevronLeft, chevronRight } from '@wordpress/icons';

/**
* Internal dependencies
Expand All @@ -28,6 +44,7 @@ import {
} from '../components/gradients';
import { cleanEmptyObject } from './utils';
import ColorPanel from './color-panel';
import InspectorControls from '../components/inspector-controls';
import useSetting from '../components/use-setting';

export const COLOR_SUPPORT_KEY = 'color';
Expand Down Expand Up @@ -207,6 +224,55 @@ function immutableSet( object, path, value ) {
return setWith( object ? clone( object ) : {}, path, value, clone );
}

function NavigationButton( {
path,
icon,
children,
isBack = false,
...props
} ) {
const navigator = useNavigator();
return (
<Item onClick={ () => navigator.push( path, { isBack } ) } { ...props }>
{ icon && (
<HStack justify="flex-start">
<FlexItem>
<Icon icon={ icon } size={ 24 } />
</FlexItem>
<FlexItem>{ children }</FlexItem>
</HStack>
) }
{ ! icon && children }
</Item>
);
}

function ScreenHeader( { back, title } ) {
return (
<VStack spacing={ 2 }>
<HStack spacing={ 2 }>
<View>
<NavigationButton
path={ back }
icon={
<Icon
icon={ isRTL() ? chevronRight : chevronLeft }
variant="muted"
/>
}
size="small"
isBack
aria-label={ __( 'Navigate to the previous view' ) }
/>
</View>
<Spacer>
<Heading level={ 5 }>{ title }</Heading>
</Spacer>
</HStack>
</VStack>
);
}

/**
* Inspector control panel containing the color related configuration
*
Expand Down Expand Up @@ -294,6 +360,24 @@ export function ColorEdit( props ) {
} else if ( hasGradientColor ) {
gradientValue = style?.color?.gradient;
}
const backgroundValue = hasBackgroundColor
? getColorObjectByAttributeValues(
allSolids,
backgroundColor,
style?.color?.background
).color
: undefined;
const textColorValue = getColorObjectByAttributeValues(
allSolids,
textColor,
style?.color?.text
).color;
const linkColorValue = hasLinkColor
? getLinkColorFromAttributeValue(
allSolids,
style?.elements?.link?.color?.text
)
: undefined;

const onChangeColor = ( name ) => ( value ) => {
const colorObject = getColorObjectByColorValue( allSolids, value );
Expand Down Expand Up @@ -371,61 +455,140 @@ export function ColorEdit( props ) {
};

return (
<ColorPanel
enableContrastChecking={
// Turn on contrast checker for web only since it's not supported on mobile yet.
Platform.OS === 'web' && ! gradient && ! style?.color?.gradient
}
clientId={ props.clientId }
settings={ [
...( hasTextColor
? [
{
label: __( 'Text color' ),
onColorChange: onChangeColor( 'text' ),
colorValue: getColorObjectByAttributeValues(
allSolids,
textColor,
style?.color?.text
).color,
},
]
: [] ),
...( hasBackgroundColor || hasGradientColor
? [
{
label: __( 'Background color' ),
onColorChange: hasBackgroundColor
? onChangeColor( 'background' )
: undefined,
colorValue: getColorObjectByAttributeValues(
allSolids,
backgroundColor,
style?.color?.background
).color,
gradientValue,
onGradientChange: hasGradientColor
? onChangeGradient
: undefined,
},
]
: [] ),
...( hasLinkColor
? [
{
label: __( 'Link Color' ),
onColorChange: onChangeLinkColor,
colorValue: getLinkColorFromAttributeValue(
allSolids,
style?.elements?.link?.color?.text
),
clearable: !! style?.elements?.link?.color
?.text,
},
]
: [] ),
] }
/>
<InspectorControls>
<PanelBody
title={ __( 'Colors' ) }
className="block-editor__hooks-colors-panel"
>
<NavigatorProvider initialPath="/">
<NavigatorScreen path="/">
<ItemGroup isBordered isSeparated>
{ ( hasBackgroundColor || hasGradientColor ) && (
<NavigationButton path="/background">
<HStack justify="flex-start">
<FlexItem>
<ColorIndicator
colorValue={
gradientValue ??
backgroundValue
}
/>
</FlexItem>
<FlexItem>
{ __( 'Background' ) }
</FlexItem>
</HStack>
</NavigationButton>
) }
{ hasTextColor && (
<NavigationButton path="/text">
<HStack justify="flex-start">
<FlexItem>
<ColorIndicator
colorValue={ textColorValue }
/>
</FlexItem>
<FlexItem>{ __( 'Text' ) }</FlexItem>
</HStack>
</NavigationButton>
) }
{ hasLinkColor && (
<NavigationButton path="/link">
<HStack justify="flex-start">
<FlexItem>
<ColorIndicator
colorValue={ linkColorValue }
/>
</FlexItem>
<FlexItem>{ __( 'Links' ) }</FlexItem>
</HStack>
</NavigationButton>
) }
</ItemGroup>
</NavigatorScreen>

{ ( hasBackgroundColor || hasGradientColor ) && (
<NavigatorScreen path="/background">
<ScreenHeader
back="/"
title={ __( 'Background' ) }
/>
<ColorPanel
enableContrastChecking={
// Turn on contrast checker for web only since it's not supported on mobile yet.
Platform.OS === 'web' &&
! gradient &&
! style?.color?.gradient
}
clientId={ props.clientId }
settings={ [
{
label: __( 'Background color' ),
onColorChange: hasBackgroundColor
? onChangeColor( 'background' )
: undefined,
colorValue: backgroundValue,
gradientValue,
onGradientChange: hasGradientColor
? onChangeGradient
: undefined,
},
] }
/>
</NavigatorScreen>
) }

{ hasTextColor && (
<NavigatorScreen path="/text">
<ScreenHeader back="/" title={ __( 'Text' ) } />
Copy link
Member

Choose a reason for hiding this comment

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

These "back" links need the ability to focus the element they represent when you go back, otherwise the top one is always selected. This also happens in global styles.

Copy link
Contributor Author

@youknowriad youknowriad Dec 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

mmm, this might not be easy without introducing some kind of #hash support in the "path" since the original element is remounted entirely.

Copy link
Member

Choose a reason for hiding this comment

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

cc @ciampo @diegohaz this seems pretty important for keyboard accessibility

Copy link
Contributor

Choose a reason for hiding this comment

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

What if the NavigatorScreen had the ability to move the focus on itself (or on a children) when mounted?

Copy link
Contributor Author

@youknowriad youknowriad Dec 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

For me the only to know the element that was focused before moving to a new "screen" is:

  • Keep a reference to the old element
  • Or have an identifier for the element.

We don't render all the screens at the same time, meaning, even if keep a reference, when going back to the previous screen, new elements will be rendered so that reference that was kept is useless.

Meaning, we only have one option: add identifiers to elements and pass the identifier somehow to navigator.push calls.

Copy link
Contributor Author

@youknowriad youknowriad Dec 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

we can explore keeping hidden screens mounted

That would be a huge performance degradation for Global Styles, there are hundreds if not thousands of screens (blocks...)

Copy link
Contributor

Choose a reason for hiding this comment

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

Would moving the focus on the new "screen" (instead that on the precise link) be an acceptable compromise? That would simplify the solution considerably (we'd just need to add the option for a screen to auto-focus when mounted)

Copy link
Member

Choose a reason for hiding this comment

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

Let's move this conversation into an issue specifically for global styles, since we might not end up doing anything here on the block inspector.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. I've opened #37063

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

<ColorPanel
enableContrastChecking={
// Turn on contrast checker for web only since it's not supported on mobile yet.
Platform.OS === 'web' &&
! gradient &&
! style?.color?.gradient
}
clientId={ props.clientId }
settings={ [
{
label: __( 'Text color' ),
onColorChange: onChangeColor( 'text' ),
colorValue: textColorValue,
},
] }
/>
</NavigatorScreen>
) }

{ hasLinkColor && (
<NavigatorScreen path="/link">
<ScreenHeader back="/" title={ __( 'Link' ) } />
<ColorPanel
enableContrastChecking={
// Turn on contrast checker for web only since it's not supported on mobile yet.
Platform.OS === 'web' &&
! gradient &&
! style?.color?.gradient
}
clientId={ props.clientId }
settings={ [
{
label: __( 'Link Color' ),
onColorChange: onChangeLinkColor,
colorValue: getLinkColorFromAttributeValue(
allSolids,
style?.elements?.link?.color?.text
),
clearable: !! style?.elements?.link
?.color?.text,
},
] }
/>
</NavigatorScreen>
) }
</NavigatorProvider>
</PanelBody>
</InspectorControls>
);
}

Expand Down
23 changes: 23 additions & 0 deletions packages/block-editor/src/hooks/color.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
.block-editor__hooks-colors-panel {

// Allow horizontal overflow so the size-increasing color indicators don't cause a scrollbar.
.components-navigator-screen {
overflow-x: visible;
}

// @todo: this can be removed when https://github.com/WordPress/gutenberg/pull/37028 lands.
.component-color-indicator {
margin-left: 0;
display: block;
border-radius: 50%;
border: 0;
height: 24px;
width: 24px;
padding: 0;
background-image:
repeating-linear-gradient(45deg, $gray-200 25%, transparent 25%, transparent 75%, $gray-200 75%, $gray-200),
repeating-linear-gradient(45deg, $gray-200 25%, transparent 25%, transparent 75%, $gray-200 75%, $gray-200);
background-position: 0 0, 25px 25px;
background-size: calc(2 * 5px) calc(2 * 5px);
}
}
1 change: 1 addition & 0 deletions packages/block-editor/src/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@
@import "./hooks/border.scss";
@import "./hooks/dimensions.scss";
@import "./hooks/typography.scss";
@import "./hooks/color.scss";

@import "./components/block-toolbar/style.scss";
@import "./components/inserter/style.scss";
Expand Down
Loading