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.

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

ColorPalette, BorderBox, BorderBoxControl: polish and DRY prop types, add default values #45463

Merged
merged 15 commits into from
Nov 3, 2022

Conversation

ciampo
Copy link
Contributor

@ciampo ciampo commented Nov 1, 2022

What?

Following up #45002 (review) and #44632, this PR refactors the types of the ColorPalette, BorderBox, and BorderBoxControl component in an effort to remove duplicate definitions, fix some wrong/missing details and improve overall code quality.

Why?

Better types = better docs and better coding experience for the developers consuming the package.

How?

  • BorderBoxControl uses types from BorderControl, and BorderControl uses types from ColorPalette. This PR removes some duplicate type defs, and instead re-uses type defs between components
  • Added default values for a few props:
    • In a few instances, the docs were mentioning a default value, but that value was not assigned in the code
    • In other instances, the default value assignment was simply moved from the component to the hook file
    • For the remaining instances, those changes are mostly done in order to align the components between themselves and offer more clarity in the docs
  • There shouldn't be any runtime changes, apart from the default value assignments

Testing Instructions

  • Check the components in Storybook, no runtime changes should be noticed compared to trunk
  • Check that docs, typescript types and README are up to date and in sync with each other

✍️ Dev Note

Many props of the ColorPalette, BorderBox and BorderBoxControl components from the @wordpress/components package have been assigned a default value which better reflects each prop's purpose.

The related docs have also been updated to reflect those changes.

@codesandbox
Copy link

codesandbox bot commented Nov 1, 2022

CodeSandbox logoCodeSandbox logo  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

@ciampo ciampo self-assigned this Nov 1, 2022
@ciampo ciampo marked this pull request as ready for review November 1, 2022 17:22
@ciampo ciampo requested a review from ajitbohra as a code owner November 1, 2022 17:22
@ciampo ciampo added [Type] Code Quality Issues or PRs that relate to code quality [Package] Components /packages/components labels Nov 1, 2022
Copy link
Contributor

@aaronrobertshaw aaronrobertshaw left a comment

Choose a reason for hiding this comment

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

Really appreciate you cleaning all this up @ciampo 🙇

✅ Code changes look good
✅ No typing errors
✅ Unit tests for each affected component are passing
✅ Components function in storybook as per trunk

Check that docs, typescript types and README are up to date and in sync with each other

There are a few small issues on this front that we might be able to clean up as well. Things like typos, descriptions not matching between README and types etc.

In case it helps, I have a diff below with what I spotted.

Click for diff
diff --git a/packages/components/src/border-box-control/border-box-control/README.md b/packages/components/src/border-box-control/border-box-control/README.md
index 7bd5989db0..ce0c3a9ebc 100644
--- a/packages/components/src/border-box-control/border-box-control/README.md
+++ b/packages/components/src/border-box-control/border-box-control/README.md
@@ -126,7 +126,7 @@ _Note: The will be `undefined` if a user clears all borders._
 
 ### `popoverPlacement`: `string`
 
-The position of the color popover relative to the control wrapper.
+The position of the color popovers relative to the control wrapper.
 
 By default, popovers are displayed relative to the button that initiated the popover. By supplying a popover placement, you force the popover to display in a specific location.
 
@@ -136,7 +136,7 @@ The available base placements are 'top', 'right', 'bottom', 'left'. Each of thes
 
 ### `popoverOffset`: `number`
 
-Works in conjunctions with `popoverPlacement` and allows leaving a space between the color popover and the control wrapper.
+The space between the popover and the control wrapper.
 
 - Required: No
 
diff --git a/packages/components/src/border-control/border-control/README.md b/packages/components/src/border-control/border-control/README.md
index 82dbdf18d1..901859df23 100644
--- a/packages/components/src/border-control/border-control/README.md
+++ b/packages/components/src/border-control/border-control/README.md
@@ -83,8 +83,7 @@ custom colors.
 
 ### `enableStyle`: `boolean`
 
-This controls whether to include border style options within the
-`BorderDropdown` sub-component.
+This controls whether to support border style selection.
 
 - Required: No
 - Default: `true`
diff --git a/packages/components/src/border-control/types.ts b/packages/components/src/border-control/types.ts
index ba1fb86f72..290618fbff 100644
--- a/packages/components/src/border-control/types.ts
+++ b/packages/components/src/border-control/types.ts
@@ -81,6 +81,12 @@ export type BorderControlProps = ColorProps &
 		 * and a close button.
 		 */
 		showDropdownHeader?: boolean;
+		/**
+		 * Size of the control.
+		 *
+		 * @default 'default'
+		 */
+		size?: 'default' | '__unstable-large';
 		/**
 		 * An object representing a border or `undefined`. Used to set the
 		 * current border configuration for this component.
@@ -96,12 +102,6 @@ export type BorderControlProps = ColorProps &
 		 * `RangeControl` for additional control over a border's width.
 		 */
 		withSlider?: boolean;
-		/**
-		 * Size of the control.
-		 *
-		 * @default 'default'
-		 */
-		size?: 'default' | '__unstable-large';
 	};
 
 export type DropdownProps = ColorProps &
diff --git a/packages/components/src/color-palette/README.md b/packages/components/src/color-palette/README.md
index 78db82c391..f5689cb0af 100644
--- a/packages/components/src/color-palette/README.md
+++ b/packages/components/src/color-palette/README.md
@@ -36,6 +36,13 @@ for the `ColorPalette`'s color swatches, by rendering your `ColorPalette` with a
 
 The component accepts the following props.
 
+### `clearable`: `boolean`
+
+Whether the palette should have a clearing button.
+
+-   Required: No
+-   Default: `true`
+
 ### `colors`: `( PaletteObject | ColorObject )[]`
 
 Array with the colors to be shown. When displaying multiple color palettes to choose from, the format of the array changes from an array of colors objects, to an array of color palettes.
@@ -45,21 +52,23 @@ Array with the colors to be shown. When displaying multiple color palettes to ch
 
 ### `disableCustomColors`: `boolean`
 
-Whether to allow the user to pick a custom color on top of the predefined choices (defined via the `colors` prop).
+Whether to allow the user to pick a custom color on top of the predefined
+choices (defined via the `colors` prop).
 
 -   Required: No
 -   Default: `false`
 
 ### `enableAlpha`: `boolean`
 
-Whether the color picker should display the alpha channel both in the bottom inputs as well as in the color picker itself.
+This controls whether the alpha channel will be offered when selecting custom
+colors.
 
 -   Required: No
 -   Default: `false`
 
 ### `value`: `string`
 
-currently active value
+Currently active value.
 
 -   Required: No
 
@@ -68,16 +77,3 @@ currently active value
 Callback called when a color is selected.
 
 -   Required: Yes
-
-### `className`: `string`
-
-classes to be applied to the container.
-
--   Required: No
-
-### `clearable`: `boolean`
-
-Whether the palette should have a clearing button.
-
--   Required: No
--   Default: `true`

@ciampo
Copy link
Contributor Author

ciampo commented Nov 2, 2022

Thank you Aaron! I've applied your suggestions, I had definitely missed a few spots.

@aaronrobertshaw aaronrobertshaw self-requested a review November 3, 2022 00:13
Copy link
Contributor

@aaronrobertshaw aaronrobertshaw left a comment

Choose a reason for hiding this comment

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

LGTM! 🚢

@ciampo ciampo merged commit 21075e1 into trunk Nov 3, 2022
@ciampo ciampo deleted the feat/border-box-control-improve-types branch November 3, 2022 12:21
@github-actions github-actions bot added this to the Gutenberg 14.6 milestone Nov 3, 2022
@bph bph mentioned this pull request Feb 6, 2023
47 tasks
@ciampo ciampo added the has dev note when dev note is done (for upcoming WordPress release) label Feb 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has dev note when dev note is done (for upcoming WordPress release) [Package] Components /packages/components [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants