BorderControl: always show Reset button #69066

shimotmk · 2025-02-05T23:00:09Z

What?

Why?

The BorderBoxControl will not display the clear button unless you set the color and style.
Also, when the clear button appears, the position of the popover changes, creating a small amount of stress.

How?

Testing Instructions

Go to post or page
Set borders for paragraph blocks, etc.
You can see that the Clear button is always visible.

Testing Instructions for Keyboard

Screenshots or screencast

Before

before.mp4

After

after.mp4

github-actions · 2025-02-05T23:00:23Z

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.

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

Co-authored-by: shimotmk <[email protected]>
Co-authored-by: ciampo <[email protected]>
Co-authored-by: Mamaduka <[email protected]>
Co-authored-by: yogeshbhutkar <[email protected]>

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

shimotmk · 2025-02-05T23:00:56Z

packages/components/src/border-control/styles.ts

@@ -147,6 +147,12 @@ export const borderControlPopoverControls = css`
 export const borderControlPopoverContent = css``;
 export const borderColorIndicator = css``;

+export const clearButton = css`


Reference

gutenberg/packages/components/src/circular-option-picker/style.scss

Lines 9 to 13 in 3f0a805

.components-circular-option-picker__custom-clear-wrapper {

display: flex;

justify-content: flex-end;

margin-top: $grid-unit-15;

}

Mamaduka · 2025-02-06T05:17:59Z

cc @WordPress/gutenberg-components

ciampo · 2025-02-06T11:00:14Z

Thank you for the ping! Here are a few thoughts:

The change is actually on BorderControl, not BorderBoxControl
We prefer working on the @wordpress/components package in isolation from the rest of the Gutenberg app for better separation of concerns and better testing (ie. unit tests, Storybook). It would be great if we could move the changes to the rest of Gutenberg to a separate PR;
Why don't we always show the existing "Reset" button? We keep it "accessible but disabled" while there are no values to reset, and otherwise enabled when there are values to reset. We can do that by reusing the logic used in the showResetButton variable, and instead of using it to conditionally render the button, we use it to conditionally disable that same button. No need to add extra props and deprecation warnings.

What do y'all think?

shimotmk · 2025-02-06T13:14:12Z

We keep it "accessible but disabled" while there are no values to reset, and otherwise enabled when there are values to reset.

I think this is a very good idea.
I was worried that some people might like the old format, so I created a process that works the same as before in the deprecated process.

If it's okay to always display the reset button, I'll delete the showClearButton props.

shimotmk · 2025-02-07T10:44:03Z

Thank you. I adjusted it.😌

ciampo · 2025-02-07T13:47:22Z

packages/components/src/border-control/border-control-dropdown/component.tsx

+				<div className={ clearButtonClassName }>
 					<Button
-						className={ resetButtonClassName }
 						variant="tertiary"
 						onClick={ () => {
 							onReset();
-							onClose();
 						} }
+						disabled={ ! showResetButton }
+						accessibleWhenDisabled
 						__next40pxDefaultSize
 					>
-						{ __( 'Reset' ) }
+						{ __( 'Clear' ) }
 					</Button>
-				</DropdownContentWrapper>
-			) }
+				</div>
+			</DropdownContentWrapper>


Are these DOM / layout changes necessary?

Adding an extra div around the reset button

moving the reset button as a child of DropdownContentWrapper

renaming and restyling the reset button to the clear button?

In other words, it seems to me that the main issue was the "reset" button being added and removed from the DOM.

If we change the logic from

{ showResetButton && ( <DropdownContentWrapper paddingSize="none"> <Button {...buttonProps} /> </DropdownContentWrapper> ) }

to

<DropdownContentWrapper paddingSize="none"> <Button {...buttonProps} accessibleWhenDisabled disabled={!showResetButton}/> </DropdownContentWrapper>

wouldn't that be enough?

Is the change from "Reset" to "Clear" necessary, including the UI/layout change?

Thanks for the review!

Are these DOM / layout changes necessary?

gutenberg/packages/components/src/circular-option-picker/circular-option-picker.tsx

Lines 165 to 169 in 3f0a805

const actions = actionsProp ? (

<div className="components-circular-option-picker__custom-clear-wrapper">

{ actionsProp }

</div>

) : undefined;

I think this is necessary to align the button to the right.
I used circular-option-picker as a reference.

renaming and restyling the reset button to the clear button?

This was also based on circular-option-picker.

@ciampo
What do you think?

I may be missing context, but was there a specific request to make it look like CircularOptionPicker?

If the main need for this PR is to fix UI jumpiness and a11y around a button being added/removed from the DOM, then I'd keep design changes out of the PR scope.

Thanks for the reply. Indeed, I changed Clear back to Reset as it was not a good idea to include it in this PR!

There are still design differences compared to what's on trunk, but I think they're probably the lowest effort we can make to keep the design working — if we kept the "reset" button to be full-width and with no left/right/bottom margins, the focus styles would be weird, and require even more hacky style overrides.

ciampo

LGTM 🚀

Let's just make sure that we rebase on top of latest trunk and make sure that CHANGELOG changes are still in the "Unreleased" > "Enhancements" section.

Reset button is looking good when enabled:

And stays in the DOM, disabled but focusable, after being clicked

A couple of notes:

the UI can still be jumpy because the "Style" section is hidden when the reset button is clicked;
it's always better to iterate and test the component in isolation, and only after in the editor; to do so, we use Storybook (npm run storybook:dev on local machines)

packages/components/src/border-control/border-control-dropdown/component.tsx

ciampo · 2025-02-14T14:02:32Z

packages/components/src/border-control/border-control-dropdown/component.tsx

+				<div className={ clearButtonClassName }>
 					<Button
-						className={ resetButtonClassName }
 						variant="tertiary"
 						onClick={ () => {
 							onReset();
-							onClose();
 						} }
+						disabled={ ! showResetButton }
+						accessibleWhenDisabled
 						__next40pxDefaultSize
 					>
-						{ __( 'Reset' ) }
+						{ __( 'Clear' ) }
 					</Button>
-				</DropdownContentWrapper>
-			) }
+				</div>
+			</DropdownContentWrapper>


There are still design differences compared to what's on trunk, but I think they're probably the lowest effort we can make to keep the design working — if we kept the "reset" button to be full-width and with no left/right/bottom margins, the focus styles would be weird, and require even more hacky style overrides.

shimotmk · 2025-02-14T21:45:19Z

@ciampo
This has been improved.
#69066 (comment)
#69066 (comment)

My research showed that a DOM change was required to style the reset button the same as the other popover components.
#69066 (comment)

Thank you.

shimotmk · 2025-02-15T21:07:16Z

@ciampo
Thanks for the review!

fix WordPress#69065

77e408b

shimotmk requested review from ajitbohra and ellatrix as code owners February 5, 2025 23:00

shimotmk commented Feb 5, 2025

View reviewed changes

Mamaduka added [Type] Enhancement A suggestion for improvement. [Feature] UI Components Impacts or related to the UI component system Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Feb 6, 2025

shimotmk added 2 commits February 7, 2025 18:14

remove showClearButton props

6a656af

test

d8c9b0a

shimotmk changed the title ~~BorderBoxControl: always show clear button~~ BorderControl: always show clear button Feb 7, 2025

shimotmk added 3 commits February 7, 2025 18:29

changelog

5ae0216

changelog

b3149c9

Merge branch 'trunk' into fix/border-box-control-clear-button

34db926

ciampo reviewed Feb 7, 2025

View reviewed changes

shimotmk added 3 commits February 8, 2025 06:26

changelog

a459bc8

clear → reset

6de075e

clear → reset

da27fb7

shimotmk changed the title ~~BorderControl: always show clear button~~ BorderControl: always show Reset button Feb 13, 2025

Merge branch 'trunk' into fix/border-box-control-clear-button

0c892d1

ciampo approved these changes Feb 14, 2025

View reviewed changes

fix

4bda128

ciampo merged commit 388d664 into WordPress:trunk Feb 15, 2025
59 checks passed

github-actions bot added this to the Gutenberg 20.4 milestone Feb 15, 2025

shimotmk deleted the fix/border-box-control-clear-button branch February 15, 2025 21:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BorderControl: always show Reset button #69066

BorderControl: always show Reset button #69066

shimotmk commented Feb 5, 2025

github-actions bot commented Feb 5, 2025 •

edited

Loading

shimotmk Feb 5, 2025

Mamaduka commented Feb 6, 2025

ciampo commented Feb 6, 2025

shimotmk commented Feb 6, 2025

shimotmk commented Feb 7, 2025

ciampo Feb 7, 2025

shimotmk Feb 7, 2025

shimotmk Feb 11, 2025

ciampo Feb 13, 2025

shimotmk Feb 13, 2025

ciampo Feb 14, 2025

ciampo left a comment

ciampo Feb 14, 2025

shimotmk commented Feb 14, 2025

shimotmk commented Feb 15, 2025

	.components-circular-option-picker__custom-clear-wrapper {
	display: flex;
	justify-content: flex-end;
	margin-top: $grid-unit-15;
	}

	const actions = actionsProp ? (
	<div className="components-circular-option-picker__custom-clear-wrapper">
	{ actionsProp }
	</div>
	) : undefined;

BorderControl: always show Reset button #69066

BorderControl: always show Reset button #69066

Conversation

shimotmk commented Feb 5, 2025

What?

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

Before

After

github-actions bot commented Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

Mamaduka commented Feb 6, 2025

ciampo commented Feb 6, 2025

shimotmk commented Feb 6, 2025

shimotmk commented Feb 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ciampo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shimotmk commented Feb 14, 2025

shimotmk commented Feb 15, 2025

github-actions bot commented Feb 5, 2025 •

edited

Loading