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

Global Styles: Caption element UI controls #44094

Closed
wants to merge 8 commits into from
Closed

Global Styles: Caption element UI controls #44094

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
aef42c3
started interface to edit caption colors in Global Styles
MaggieCabrera Sep 12, 2022
b96ce01
removed gradient variable for now
MaggieCabrera Sep 15, 2022
dace6c1
removed class from ui headings
MaggieCabrera Sep 15, 2022
525c104
add typography supports
MaggieCabrera Sep 15, 2022
498f64f
abstract the color caption component to work for all elements
MaggieCabrera Oct 11, 2022
cba3235
refactor button color component into element
MaggieCabrera Oct 11, 2022
80a3760
refactored text into element component
MaggieCabrera Oct 11, 2022
8094107
removed headings and links from the elements component
MaggieCabrera Oct 14, 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
102 changes: 0 additions & 102 deletions packages/edit-site/src/components/global-styles/screen-button-color.js
View file
Open in desktop

This file was deleted.

143 changes: 143 additions & 0 deletions packages/edit-site/src/components/global-styles/screen-color-element.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
/**
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { __experimentalColorGradientControl as ColorGradientControl } from '@wordpress/block-editor';
/**
* Internal dependencies
*/
import ScreenHeader from './header';
import {
getSupportedGlobalStylesPanels,
useSetting,
useStyle,
useColorsPerOrigin,
} from './hooks';

const elements = {
text: {
description: __(
'Set the default color used for text across the site.'
),
title: __( 'Text' ),
},
caption: {
description: __( 'Set the colors used for captions across the site' ),
title: __( 'Captions' ),
},
button: {
description: __(
'Set the default colors used for buttons across the site.'
),
title: __( 'Buttons' ),
},
};

function ScreenColorElement( { name, element } ) {
const supports = getSupportedGlobalStylesPanels( name );
const colorsPerOrigin = useColorsPerOrigin( name );
const [ solids ] = useSetting( 'color.palette', name );
const [ areCustomSolidsEnabled ] = useSetting( 'color.custom', name );
let [ isBackgroundEnabled ] = useSetting( 'color.background', name );
const [ isTextEnabled ] = useSetting( 'color.text', name );

let textColorElementSelector = 'elements.' + element + '.color.text';
const backgroundColorElementSelector =
'elements.' + element + '.color.background';

let hasElementColor = false;

switch ( element ) {
case 'button':
hasElementColor =
supports.includes( 'buttonColor' ) &&
isBackgroundEnabled &&
( solids.length > 0 || areCustomSolidsEnabled );
break;
case 'text':
hasElementColor =
supports.includes( 'color' ) &&
isTextEnabled &&
( solids.length > 0 || areCustomSolidsEnabled );
textColorElementSelector = 'color.text';
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the only place we mutate textColorElementSelector? If so, would it work to do the calculation when we define that variable, and switch to a const?

Alternately, what if we moved the selector to be a key of the element in the elements object above (so, elements.text.textColorElementSelector?

isBackgroundEnabled = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly if we are hard-coding particular elements to disable certain features, would it work to add it to the elements object?

break;

default:
hasElementColor =
supports.includes( 'color' ) &&
( solids.length > 0 || areCustomSolidsEnabled );
break;
}
Comment on lines +50 to +71
Copy link
Contributor

Choose a reason for hiding this comment

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

This is potentially a little nit-picky, and it doesn't have any impact on the overall functioning of this code, so feel free to ignore this comment!

Some of the global styles files get complex pretty quickly as we add additional elements and controls, and looking at the changes here made me think of a couple of small changes that might improve readability. Is there an opportunity here to add a useHasElementColor hook to this file, similar to how the border-panel.js and dimensions-panel.js files have a bunch of useHas hooks?

We could then potentially swap out the switch statement to use that hook, where the internals might be a shorter series of if statements:

	if ( solids.length > 0 || areCustomSolidsEnabled ) {
		if ( element === 'button' ) {
			hasElementColor =
				supports.includes( 'buttonColor' ) && isBackgroundEnabled;
		} else if ( element === 'text' ) {
			hasElementColor = supports.includes( 'color' ) && isTextEnabled;
		} else {
			hasElementColor = supports.includes( 'color' );
		}
	}

Ideally ScreenColorElement would then either get its settings via calling other hooks, or do a look-up to the elements object.

My feedback here is very much influenced by my personal preference that I find switch statements challenging to read, so definitely not a blocker to this PR landing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Breaking this into a separate hook sounds good to me 👍

Along with making it easier to grok. It might open the door for writing tests as well, should we ever desire them.

My feedback here is very much influenced by my personal preference that I find switch statements challenging to read

For me, I think it might be the nested and repeated check for ( solids.length > 0 || areCustomSolidsEnabled ) that lowers the readability more so than the switch. That's just another personal opinion or preference though 🙂


const [ elementTextColor, setElementTextColor ] = useStyle(
textColorElementSelector,
name
);

const [ userElementTextColor ] = useStyle(
textColorElementSelector,
name,
'user'
);

const [ elementBgColor, setElementBgColor ] = useStyle(
backgroundColorElementSelector,
name
);

const [ userElementBgColor ] = useStyle(
backgroundColorElementSelector,
name,
'user'
);

if ( ! hasElementColor ) {
return null;
}

return (
<>
<ScreenHeader
title={ elements[ element ].title }
description={ elements[ element ].description }
/>
<div className="edit-site-global-styles-screen-element-color">
<h4>{ __( 'Text color' ) }</h4>

<ColorGradientControl
className="edit-site-screen-element-color__control"
colors={ colorsPerOrigin }
disableCustomColors={ ! areCustomSolidsEnabled }
__experimentalHasMultipleOrigins
showTitle={ false }
enableAlpha
__experimentalIsRenderedInSidebar
colorValue={ elementTextColor }
onColorChange={ setElementTextColor }
clearable={ elementTextColor === userElementTextColor }
/>
</div>
{ isBackgroundEnabled && (
<div className="edit-site-global-styles-screen-element-color">
<h4>{ __( 'Background color' ) }</h4>

<ColorGradientControl
className="edit-site-screen-element-color__control"
colors={ colorsPerOrigin }
disableCustomColors={ ! areCustomSolidsEnabled }
__experimentalHasMultipleOrigins
showTitle={ false }
enableAlpha
__experimentalIsRenderedInSidebar
colorValue={ elementBgColor }
onColorChange={ setElementBgColor }
clearable={ elementBgColor === userElementBgColor }
/>
</div>
) }
</>
);
}

export default ScreenColorElement;
34 changes: 34 additions & 0 deletions packages/edit-site/src/components/global-styles/screen-colors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,36 @@ function LinkColorItem( { name, parentMenu } ) {
);
}

function CaptionColorItem( { name, parentMenu } ) {
const supports = getSupportedGlobalStylesPanels( name );
const hasSupport = supports.includes( 'color' );
const [ color ] = useStyle( 'elements.caption.color.text', name );
const [ bgColor ] = useStyle( 'elements.caption.color.background', name );

if ( ! hasSupport ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also skip the panel if a theme author has chosen to disable text and background color controls in Global Styles?

I appreciate this follows the other elements that miss this as well. So it would likely be better as a follow-up.

return null;
}

return (
<NavigationButtonAsItem
path={ parentMenu + '/colors/caption' }
aria-label={ __( 'Colors caption styles' ) }
>
<HStack justify="flex-start">
<ZStack isLayered={ false } offset={ -8 }>
<ColorIndicatorWrapper expanded={ false }>
<ColorIndicator colorValue={ bgColor } />
</ColorIndicatorWrapper>
<ColorIndicatorWrapper expanded={ false }>
<ColorIndicator colorValue={ color } />
</ColorIndicatorWrapper>
</ZStack>
<FlexItem>{ __( 'Captions' ) }</FlexItem>
</HStack>
</NavigationButtonAsItem>
);
}

function HeadingColorItem( { name, parentMenu } ) {
const supports = getSupportedGlobalStylesPanels( name );
const hasSupport = supports.includes( 'color' );
Expand Down Expand Up @@ -204,6 +234,10 @@ function ScreenColors( { name } ) {
name={ name }
parentMenu={ parentMenu }
/>
<CaptionColorItem
name={ name }
parentMenu={ parentMenu }
/>
<HeadingColorItem
name={ name }
parentMenu={ parentMenu }
Expand Down
62 changes: 0 additions & 62 deletions packages/edit-site/src/components/global-styles/screen-text-color.js
View file
Open in desktop

This file was deleted.

4 changes: 4 additions & 0 deletions packages/edit-site/src/components/global-styles/screen-typography-element.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ const elements = {
description: __( 'Manage the fonts and typography used on headings.' ),
title: __( 'Headings' ),
},
caption: {
description: __( 'Manage the fonts and typography used on captions.' ),
title: __( 'Captions' ),
},
button: {
description: __( 'Manage the fonts and typography used on buttons.' ),
title: __( 'Buttons' ),
Expand Down
6 changes: 6 additions & 0 deletions packages/edit-site/src/components/global-styles/screen-typography.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,12 @@ function ScreenTypography( { name } ) {
element="heading"
label={ __( 'Headings' ) }
/>
<Item
name={ name }
parentMenu={ parentMenu }
element="caption"
label={ __( 'Captions' ) }
/>
<Item
name={ name }
parentMenu={ parentMenu }
Expand Down
1 change: 1 addition & 0 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 @@ -26,6 +26,7 @@
max-width: 100%;
}

.edit-site-global-styles-screen-element-color,
.edit-site-global-styles-screen-heading-color,
.edit-site-global-styles-screen-typography {
margin: $grid-unit-20;
Expand Down
Loading