ColorPalette: don't render when custom colors disabled and no colors passed

[n8finch]

First PR for Gutenberg, woohoo!

What?

Closes #59567

This PR fixes the disableCustomColors prop that was not being set in the BorderBoxControl element.

Why?

If the custom color setting is disabled in the theme.json the BorderBoxControl element was not picking up on that setting.

The setting was respected when picking text and background colors though, as these use the ColorPanelDropdown element.

How?

This PR adds a local method to the BorderBoxControl element and checks for the theme setting in a similar manner that other settings are being checked and set.

I've tried to make it as close as possible to the implementation in the ColorPanelDropdown element, see related code:

• getting the value: https://github.com/WordPress/gutenberg/blob/trunk/packages/block-editor/src/components/global-styles/color-panel.js#L332
• passing the value: https://github.com/WordPress/gutenberg/blob/trunk/packages/block-editor/src/components/global-styles/color-panel.js#L742

Testing Instructions

1. Make sure you're working with a custom theme.json file, and that you have the settings.color.custom property set to false (see screenshots)
2. Open the Outline for the block editor
3. Insert or select a Group Block
4. Open the right sidebar Inspector
5. Scroll to the "Border & Shadow" panel section
6. Click on the Border button trigger (see screenshots)
7. You should not see the custom color picker
8. Change the value in the theme.json to be true or remove it completely, reload the page, and repeat steps 3 to 6, at this point you should see the custom color picker.

Testing Instructions for Keyboard

1. Make sure you're working with a custom theme.json file, and that you have the settings.color.custom property set to false (see screenshots)
2. Tab to select the group block in the block editor or Outline
3. Open the Inspector with CMD + Shift + , on a Mac or Ctrl + Shift + , on a PC.
4. Tab until you get to the Border button trigger labeled Border color and style picker
5. Hit Enter or Spacebar on your keyboard
6. You should not be able to see or select the custom color picker
7. Change the value in the theme.json to be true or remove it completely, reload the page, and repeat steps 3 to 6, at this point you should see the custom color picker.

Screenshots or screencast

Before	After

Also note that it respects setting to true and if it's removed

True	Removed

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @n8finch@git.wordpress.org, @snibe.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Unlinked contributors: n8finch@git.wordpress.org, snibe.

Co-authored-by: n8finch <n8finch@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org>
Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: ramonjd <ramonopoly@git.wordpress.org>
Co-authored-by: annezazu <annezazu@git.wordpress.org>
Co-authored-by: cbravobernal <cbravobernal@git.wordpress.org>
Co-authored-by: vraja-pro <vrajadas@git.wordpress.org>
Co-authored-by: andreawetzel <awetz583@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[github-actions]

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @n8finch! In case you missed it, we'd love to have you join us in our Slack community.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

[n8finch]

@ellatrix and @ajitbohra would love to get an initial review on this as it's been sitting around a while. I just updated the branch so it could be merged. Thanks so much! 🙂

[t-hamano]

Thanks for the PR! Overall, it looks good. However, if both custom color and color preset are disabled, an empty div is rendered within the popover, causing redundant space. Please try the following theme.json:

{
	"$schema": "../../schemas/json/theme.json",
	"version": 3,
	"settings": {
		"appearanceTools": true,
		"layout": {
			"contentSize": "840px",
			"wideSize": "1100px"
		},
		"color": {
			"defaultPalette": false,
			"custom": false
		}
	}
}

If there are no colors at all, the rendering of this div should be prevented.

diff --git a/packages/components/src/border-control/border-control-dropdown/component.tsx b/packages/components/src/border-control/border-control-dropdown/component.tsx
index c02da4d8ada..51c035fba68 100644
--- a/packages/components/src/border-control/border-control-dropdown/component.tsx
+++ b/packages/components/src/border-control/border-control-dropdown/component.tsx
@@ -203,17 +203,19 @@ const BorderControlDropdown = (
        const renderContent = () => (
                <DropdownContentWrapper paddingSize="medium">
                        <VStack className={ popoverControlsClassName } spacing={ 6 }>
-                               <ColorPalette
-                                       className={ popoverContentClassName }
-                                       value={ color }
-                                       onChange={ onColorChange }
-                                       { ...{ colors, disableCustomColors } }
-                                       __experimentalIsRenderedInSidebar={
-                                               __experimentalIsRenderedInSidebar
-                                       }
-                                       clearable={ false }
-                                       enableAlpha={ enableAlpha }
-                               />
+                               { ( colors.length > 0 || ! disableCustomColors ) && (
+                                       <ColorPalette
+                                               className={ popoverContentClassName }
+                                               value={ color }
+                                               onChange={ onColorChange }
+                                               { ...{ colors, disableCustomColors } }
+                                               __experimentalIsRenderedInSidebar={
+                                                       __experimentalIsRenderedInSidebar
+                                               }
+                                               clearable={ false }
+                                               enableAlpha={ enableAlpha }
+                                       />
+                               ) }
                                { enableStyle && isStyleSettable && (
                                        <BorderControlStylePicker
                                                label={ __( 'Style' ) }

[n8finch]

@t-hamano thank you for the quick review and help with the wrapper there. I've got the code update. Let me know if it needs anything else. 🙌

[ramonjd]

This is testing well for me after the latest update.

The CI is complaining about this prop:

BorderBoxControl should have the __next40pxDefaultSize prop when using the default size.

[aaronrobertshaw]

Thanks for all the work and iterations here 🙇

This tests pretty well for me too. I left a couple of inline comments but the only thing I think we should address before looking to merge would be extending the test coverage to also cover the "no color options" scenario as well.

[aaronrobertshaw]

We should add a test to cover excluding the ColorPalette when there are no colors.

[n8finch]

@aaronrobertshaw got a test added for that.

[aaronrobertshaw]

Thanks for the quick iteration @n8finch. Unfortunately, this test doesn't actually cover the change.

The guard you added prevents an empty ColorPalette wrapper from rendering when there are no colors and custom colors are disabled. This new test though only asserts that the "Custom color picker" button is absent. That button is never present when disableCustomColors is true regardless of this fix, so the test passes with or without your change and provides no regression protection.

[n8finch]

@aaronrobertshaw right, ok, there are four tests now covering:

• custom color picker should be enabled
• custom color picker should be disabled
• the colors array has colors and should show the palette pickers
• the colors array does not have colors and should not show the palette pickers

So that will check both sides of the guard now.

[aaronrobertshaw]

@n8finch unfortunately, the tests still do not cover the guards behaviour.

The guard prevents rendering the entire ColorPalette not just the custom color picker. So as it stands, the tests still don't protect against a regression here.

Also, I missed this on my earlier review but the tests here are for the BorderBoxControl not the BorderControl component that has actually been updated. That probably needs addressing too.

[n8finch]

A lot has changed recently and there's now a conflict on this branch, so we'll need a quick rebase.

The tests also need some tweaking. I've left further inline comments towards this end.

@aaronrobertshaw ok, got the branch rebased, so, good there.

I've moved the tests to the BorderColor test suite. There's already tests there that check things render when colors are present, so that's good. I added two tests:

• one to make sure things don't render if colors is empty and disableCustomColors is true
• another to make sure that the color circles render but not the custom color picker if disableCustomColors is true

Thank you for the back and forth!

[aaronrobertshaw]

Thanks, I'll be able to review properly on Monday but for now there are a few linting errors that need cleaning up.

[ciampo]

A few notes on my end, too:

• we'll need a CHANGELOG entry;
• as also flagged by @aaronrobertshaw , the added tests don't really exercise the guard, and don't protect against regressions (they assert the element is absent, regardless of the fix)

But most importantly, it looks like the logic introduced in packages/components/src/border-control/border-control-dropdown/component.tsx is duplicating ColorPalette logic:

gutenberg/packages/components/src/color-palette/index.tsx

Line 281 in dfb650e

{ ! disableCustomColors && (

It seems to me that the better fix is to have ColorPalette return null instead of an empty VStack ?

[n8finch]

A few notes on my end, too:

• we'll need a CHANGELOG entry;
• as also flagged by @aaronrobertshaw , the added tests don't really exercise the guard, and don't protect against regressions (they assert the element is absent, regardless of the fix)

But most importantly, it looks like the logic introduced in packages/components/src/border-control/border-control-dropdown/component.tsx is duplicating ColorPalette logic:

gutenberg/packages/components/src/color-palette/index.tsx

Line 281 in dfb650e

{ ! disableCustomColors && (

It seems to me that the better fix is to have ColorPalette return null instead of an empty VStack ?

Thanks @ciampo for the review. I've:

• updated the changelog
• changed the logic to check in the ColorPalette component and return null so there's no empty VStack
• made sure the tests pass.

Note on the tests: I've added only the tests that were needed to check that the custom color picker and circular color options do not show if they're opted out of or there are no colors, there are already tests above that check to make sure these items render see: it( 'should render color and style popover'...:

gutenberg/packages/components/src/border-control/test/index.js

Lines 136 to 161 in dfb650e

	it( 'should render color and style popover', async () => {
	const user = userEvent.setup();
	const props = createProps();
	render( <TestBorderControl { ...props } /> );
	await openPopover( user );
	const customColorPicker = getButton( /Custom color picker/ );
	const circularOptionPicker = screen.getByRole( 'listbox', {
	name: 'Custom color picker',
	} );
	const colorSwatchButtons =
	within( circularOptionPicker ).getAllByRole( 'option' );
	const styleLabel = screen.getByText( 'Style' );
	const solidButton = getButton( 'Solid' );
	const dashedButton = getButton( 'Dashed' );
	const dottedButton = getButton( 'Dotted' );
	const resetButton = getButton( 'Reset' );
	expect( customColorPicker ).toBeInTheDocument();
	expect( colorSwatchButtons.length ).toEqual( colors.length );
	expect( styleLabel ).toBeInTheDocument();
	expect( solidButton ).toBeInTheDocument();
	expect( dashedButton ).toBeInTheDocument();
	expect( dottedButton ).toBeInTheDocument();
	expect( resetButton ).toBeInTheDocument();
	} );

[ciampo]

LGTM 🚀

We'll be able to merge once the remaining feedback is addressed, and conflicts with trunk are solved.

Thank you!

[n8finch]

LGTM 🚀

We'll be able to merge once the remaining feedback is addressed, and conflicts with trunk are solved.

Thank you!

@ciampo Thanks for your help! I think (hope!) everything is good here.

[ramonjd]

I can merge today. Thanks everyone for the feedback, and the continued iteration @n8finch