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

Jump to bottom

BorderControl: Render border color/style dropdown as UnitControl prefix #42212

Merged
merged 4 commits into from
Jul 22, 2022
Merged

BorderControl: Render border color/style dropdown as UnitControl prefix #42212

merged 4 commits into from
Jul 22, 2022

Conversation

aaronrobertshaw
Copy link
Contributor

What?

Renders the BorderControl dropdown as a prefix within its UnitControl.

Why?

Rendering as a prefix on the UnitControl allows us to reduce some of the CSS overrides.

How?

  1. Fixes UnitControlProps type to allow a ReactNode for the prefix prop.
  2. Renders the existing border control dropdown via the UnitControl's prefix.
  3. Removes the obsolete HStack wrapper and cleans up/fixes styles

Testing Instructions

  1. Ensure there are no type errors: npm run build:package-types
  2. Check BorderControl unit tests still pass: npm run test-unit packages/components/src/border-control/test
  3. Confirm BorderBoxControl unit tests pass: npm run test-unit packages/components/src/border-box-control/test
  4. Load Storybook and confirm no visual regressions for both BorderControl & BorderBoxControl
  5. Switch to the block editor, edit a post, add a group block, and select it. Ensure it looks and functions as it should.

@aaronrobertshaw aaronrobertshaw added [Type] Enhancement A suggestion for improvement. [Feature] UI Components Impacts or related to the UI component system [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi labels Jul 7, 2022
@aaronrobertshaw aaronrobertshaw self-assigned this Jul 7, 2022
@aaronrobertshaw aaronrobertshaw marked this pull request as ready for review July 7, 2022 06:24
@aaronrobertshaw aaronrobertshaw mentioned this pull request Jul 7, 2022
Merged
andrewserong
andrewserong approved these changes Jul 8, 2022
View reviewed changes
Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

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

Nice cleanup PR @aaronrobertshaw! This is testing well for me in Storybook and the different widths (compact and custom width), and the component still looks and functions the same in the editor via a Group block and the border controls.

LGTM! ✨

packages/components/src/unit-control/types.ts Show resolved Hide resolved
@aaronrobertshaw aaronrobertshaw requested a review from mirka July 8, 2022 07:06
mirka
mirka reviewed Jul 8, 2022
View reviewed changes
Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

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

I'm very happy to see things getting simplified 🎉

packages/components/src/border-control/styles.ts Outdated
Comment on lines 36 to 40
/* Prevent unit select forcing min height larger than its UnitControl */
min-height: 0;
${ rtl(
{
borderRadius: '0 1px 1px 0',
marginRight: 0,
},
{
borderRadius: '1px 0 0 1px',
marginLeft: 0,
}
{ borderRadius: '0 2px 2px 0' },
{ borderRadius: '2px 0 0 2px' }
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand what these styles are for 🤔 Are they no longer needed...?

The :focus stuff too, should they be moved to UnitControl itself? (Not necessarily in this PR)

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 swear that when I was putting this PR together I still needed these styles or the unit select showed square corners. I must have been seeing things. After retesting the border-radius rules can be removed.

While testing the focus styles it looks like the focus styling in general for the UnitControl is a little broken e.g. wrapper's border overlays the unit selects focus border/box-shadow etc.

I've moved these focus styles to the UnitControl in a separate PR in the hope that might provide a better historical record if we need to revisit them in the future. See #42383.

For this PR, I'll update it shortly to target the #42383 branch as its base and simply remove the overrides from the BorderControl.

packages/components/src/border-control/styles.ts Outdated Show resolved Hide resolved
packages/components/src/border-control/styles.ts Outdated Show resolved Hide resolved
@ciampo ciampo added the [Package] Components /packages/components label Jul 11, 2022
@aaronrobertshaw aaronrobertshaw mentioned this pull request Jul 13, 2022
Merged
@aaronrobertshaw aaronrobertshaw force-pushed the refactor/border-control-dropdown branch 2 times, most recently from 41dce38 to ffc8d0d Compare July 13, 2022 07:02
@aaronrobertshaw aaronrobertshaw changed the base branch from trunk to update/unit-control-select-focus-styling July 13, 2022 07:04
@andreszs
Copy link

Nice PR, too bad it was ignored by the 6.0.1 update from earlier today.

ciampo
ciampo approved these changes Jul 14, 2022
View reviewed changes
Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

Great work!

packages/components/src/border-control/styles.ts Outdated Show resolved Hide resolved
@aaronrobertshaw aaronrobertshaw force-pushed the update/unit-control-select-focus-styling branch from 2cfb9f2 to 4a6a3e6 Compare July 15, 2022 05:02
Base automatically changed from update/unit-control-select-focus-styling to trunk July 15, 2022 05:51
@aaronrobertshaw aaronrobertshaw force-pushed the refactor/border-control-dropdown branch from ffc8d0d to 9974f64 Compare July 15, 2022 08:01
@aaronrobertshaw aaronrobertshaw force-pushed the refactor/border-control-dropdown branch from 9974f64 to 498d1df Compare July 22, 2022 01:36
@aaronrobertshaw aaronrobertshaw merged commit 73c856a into trunk Jul 22, 2022
@aaronrobertshaw aaronrobertshaw deleted the refactor/border-control-dropdown branch July 22, 2022 03:31
@github-actions github-actions bot added this to the Gutenberg 13.8 milestone Jul 22, 2022
@aaronrobertshaw aaronrobertshaw mentioned this pull request Jul 25, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mirka mirka mirka left review comments

@andrewserong andrewserong andrewserong approved these changes

@ciampo ciampo ciampo approved these changes

@ajitbohra ajitbohra Awaiting requested review from ajitbohra ajitbohra is a code owner

Assignees

@aaronrobertshaw aaronrobertshaw

Labels
[Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi [Feature] UI Components Impacts or related to the UI component system [Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
None yet
Milestone
Gutenberg 13.8
Development

Successfully merging this pull request may close these issues.
5 participants
@aaronrobertshaw @andreszs @mirka @ciampo @andrewserong