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

Convert the ColorPalette component to TypeScript #44632

Merged
merged 43 commits into from
Oct 26, 2022

Conversation

kienstra
Copy link
Contributor

@kienstra kienstra commented Oct 2, 2022

What?

Convert the ColorPalette component to TypeScript

Why?

As part of an effort to convert @wordpress/components to TypeScript

How?

By adding types to the ColorPalette component and its story.

Testing Instructions

  1. npm run storybook:dev
  2. Verify that the ColorPalette component works the same as before

Screenshots or screencast

color-palette

Potential follow-ups

  • Refactor all styles to Emotion
  • Improve APIs — currently __experimentalHasMultipleOrigins relies on the fact that the format of color is correct. What if we inferred the single/multiple palette based on the format of color instead?

    There are some ts-ignore statements
    that would be good to remove.
@kienstra kienstra marked this pull request as ready for review October 2, 2022 22:25
@kienstra kienstra requested a review from ajitbohra as a code owner October 2, 2022 22:25
@kienstra
Copy link
Contributor Author

kienstra commented Oct 3, 2022

Hi @ciampo, would you have time to review this?

You probably have a ton on your plate, thanks for any time you can give it.

Thanks, Marco!

@ciampo ciampo requested review from mirka, ciampo and chad1008 October 5, 2022 08:35
@ciampo ciampo added [Type] Enhancement A suggestion for improvement. [Feature] UI Components Impacts or related to the UI component system [Package] Components /packages/components labels Oct 5, 2022
@kienstra
Copy link
Contributor Author

Hi @ajitbohra,
Would you happen to have time to review this? You're probably really busy, so thanks for whatever you can spend on this.

Thanks, Ajit!

@ciampo
Copy link
Contributor

ciampo commented Oct 12, 2022

Hey @kienstra , I will try to review this today. I would normally be much faster, but I've been AFK last week and I'm still catching up — apologies!

In the meantime, I just wanted to thank you for submitting this PR in the first place!

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.

Hey @kienstra , it's me again! Sorry for the delay and thank you for your patience. I'm currently part of the squad that maintains the @wordpress/components package, so you've definitely tagged the right person to review this PR! Out of curiosity — how did you come across the TypeScript migration?

I've started taking a look at ColorPalette, and I must say that this is a tricky component to refactor (mostly due to a few debatable API choices around the single/multiple palettes).

From a quick look, there are many aspects that we need to refine, and therefore I've decided that I will approach this PR review in multiple review passes, and with each iteration we'll get closer and closer to being able to merge it.

Here's a first list of what I'm planning on focusing:

  • first, I will try to help you to iron out all the @ts-ignore comments that are left in code
  • after that, we'll focus on reviewing the existing types and using (if possible) the WordPressComponentProps type
  • after that, I will help you to refactor any missing files to TypeScript (eg. styles, unit tests..)
  • after that, I will make sure that the component still functions correctly (and potentially suggest any changes / new tests)
  • finally, we will focus on Storybook and the component's docs (both in types.ts/Storybook, and README)

On my first review pass, I focused on getting rid of a few @ts-ignore comments, starting with the ones in the BorderControl component and the ones in ColorPalette caused by the single vs multiple colors duality.

Here's what I noticed:

  • From existing code, it looks like ColorPalette actually expects that colors can be undefined
  • The other main type mismatch between ColorPalette and BorderControl is that the color property is defined as CSSProperties[ 'color' ] (which also includes undefined), while that same property is defined as of type string in ColorPalette.
  • The fact that the MultipleColors type is currently defined as an array seems to cause some issues in TypeScript
  • The current logic in ColorPalette around showMultiplePalettes is a bit messy and not very robust:
    • it doesn't allow TypeScript to properly infer the correct type of colors when passed to Component
    • what if colors has a single palette format, but the __experimentalHasMultipleOrigins prop is set to true?
Following the observations above, I was able to make a few changes to the code that seem to make the code a bit more tidy and remove the need for a couple of `@ts-ignore` (click to expand)
  • I aligned the type of the color property between BorderControl and ColorPalette — they are now both a NonNullable< CSSProperties[ 'color' ] >
  • I removed the array type notation from MultipleColors, and added it inline where needed. This also allowed me to write the colors type as ( MultipleColors | Color )[], which seems to help TypeScript to properly infer it as an array at all times
  • I marked the colors prop on ColorPalette as non-required, and added instead a few checks inside ColorPalette components to make sure that the palette UI is not rendered when the list of colors is empty
  • I've added a areColorsMultiplePalette utility function, and used it to make sure that the format of colors is compatible with the value of the __experimentalHasMultipleOrigins prop. When the values are not compatible, the component doesn't render anything and outputs a warning (normally we wouldn't introduce such a runtime change in a TypeScript refactor, but this particular change allows us to make TypeScript happy AND fixes a potential bug. We can decided at a later point if we want to split this change in a follow-up PR).
  • I refactored the logic around how single/multiple palettes are rendered. Instead of using a generic Component, I'm calling MultiplePalettes and SinglePalette explicitly, which allows us to typecast colors accordingly (this is made possible by the change from the previous point, which ensures that the format of colors is indeed matching the __experimentalHasMultipleOrigins prop)
  • I also refactored some types in the color-palette/index.tsx file to reuse the ColorPaletteProps prop types, instead of re-declaring them as new types
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 945d2ae9d6..bfe442f4c6 100644
--- a/packages/components/src/border-control/border-control-dropdown/component.tsx
+++ b/packages/components/src/border-control/border-control-dropdown/component.tsx
@@ -198,7 +198,6 @@ const BorderControlDropdown = (
 							/>
 						</HStack>
 					) : undefined }
-					{ /* @ts-ignore colors prop: Type 'Colors | undefined' is not assignable to type 'Color[] | MultipleColors' */ }
 					<ColorPalette
 						className={ popoverContentClassName }
 						value={ color }
diff --git a/packages/components/src/border-control/types.ts b/packages/components/src/border-control/types.ts
index 20cef1e4f6..a0330de20e 100644
--- a/packages/components/src/border-control/types.ts
+++ b/packages/components/src/border-control/types.ts
@@ -16,7 +16,7 @@ export type Border = {
 
 export type Color = {
 	name: string;
-	color: CSSProperties[ 'color' ];
+	color: NonNullable< CSSProperties[ 'color' ] >;
 };
 
 export type ColorOrigin = {
diff --git a/packages/components/src/color-palette/index.tsx b/packages/components/src/color-palette/index.tsx
index 8e72f4a56e..2330d8f61e 100644
--- a/packages/components/src/color-palette/index.tsx
+++ b/packages/components/src/color-palette/index.tsx
@@ -82,6 +82,11 @@ function SinglePalette( {
 			);
 		} );
 	}, [ colors, value, onChange, clearColor ] );
+
+	if ( colors.length === 0 ) {
+		return null;
+	}
+
 	return (
 		// @ts-ignore Required children prop.
 		<CircularOptionPicker
@@ -100,6 +105,10 @@ function MultiplePalettes( {
 	value,
 	actions,
 }: MultiplePalettesProps ) {
+	if ( colors.length === 0 ) {
+		return null;
+	}
+
 	return (
 		<VStack spacing={ 3 } className={ className }>
 			{ colors.map( ( { name, colors: colorPalette }, index ) => {
@@ -157,9 +166,9 @@ export function CustomColorPickerDropdown( {
 }
 
 export const extractColorNameFromCurrentValue = (
-	currentValue?: string,
-	colors: Color[] | MultipleColors = [],
-	showMultiplePalettes: boolean = false
+	currentValue?: ColorPaletteProps[ 'value' ],
+	colors: ColorPaletteProps[ 'colors' ] = [],
+	showMultiplePalettes: ColorPaletteProps[ '__experimentalHasMultipleOrigins' ] = false
 ) => {
 	if ( ! currentValue ) {
 		return '';
@@ -171,8 +180,9 @@ export const extractColorNameFromCurrentValue = (
 		: colord( currentValue ).toHex();
 
 	// Normalize format of `colors` to simplify the following loop
-	const colorPalettes = showMultiplePalettes
-		? ( colors as MultipleColors )
+	type normalizedPaletteObject = { colors: Color[] };
+	const colorPalettes: normalizedPaletteObject[] = showMultiplePalettes
+		? ( colors as MultipleColors[] )
 		: [ { colors: colors as Color[] } ];
 	for ( const { colors: paletteColors } of colorPalettes ) {
 		for ( const { name: colorName, color: colorValue } of paletteColors ) {
@@ -197,10 +207,18 @@ export const showTransparentBackground = ( currentValue?: string ) => {
 	return colord( currentValue ).alpha() === 0;
 };
 
+const areColorsMultiplePalette = (
+	colors: NonNullable< ColorPaletteProps[ 'colors' ] >
+): colors is MultipleColors[] => {
+	return colors.every( ( colorObj ) =>
+		Array.isArray( ( colorObj as MultipleColors ).colors )
+	);
+};
+
 export default function ColorPalette( {
 	clearable = true,
 	className,
-	colors,
+	colors = [],
 	disableCustomColors = false,
 	enableAlpha,
 	onChange,
@@ -209,9 +227,31 @@ export default function ColorPalette( {
 	__experimentalIsRenderedInSidebar = false,
 }: ColorPaletteProps ) {
 	const clearColor = useCallback( () => onChange( undefined ), [ onChange ] );
-	const showMultiplePalettes =
-		__experimentalHasMultipleOrigins && !! colors?.length;
-	const Component = showMultiplePalettes ? MultiplePalettes : SinglePalette;
+
+	const buttonLabelName = useMemo(
+		() =>
+			extractColorNameFromCurrentValue(
+				value,
+				colors,
+				__experimentalHasMultipleOrigins
+			),
+		[ value, colors, __experimentalHasMultipleOrigins ]
+	);
+
+	// Make sure that the `colors` array has a format (single/multiple) that is
+	// compatible with the `__experimentalHasMultipleOrigins` flag. This is true
+	// when __experimentalHasMultipleOrigins and areColorsMultiplePalette() are
+	// either both `true` or both `false`.
+	if (
+		colors.length > 0 &&
+		__experimentalHasMultipleOrigins !== areColorsMultiplePalette( colors )
+	) {
+		// eslint-disable-next-line no-console
+		console.warn(
+			'wp.components.ColorPalette: please specify a format for the `colors` prop that is compatible with the `__experimentalHasMultipleOrigins` prop.'
+		);
+		return null;
+	}
 
 	const renderCustomColorPicker = () => (
 		<DropdownContentWrapper paddingSize="none">
@@ -228,15 +268,6 @@ export default function ColorPalette( {
 	const valueWithoutLeadingHash = value?.startsWith( '#' )
 		? value.substring( 1 )
 		: value ?? '';
-	const buttonLabelName = useMemo(
-		() =>
-			extractColorNameFromCurrentValue(
-				value,
-				colors,
-				showMultiplePalettes
-			),
-		[ value, colors, showMultiplePalettes ]
-	);
 
 	const customColorAccessibleLabel = !! valueWithoutLeadingHash
 		? sprintf(
@@ -249,6 +280,19 @@ export default function ColorPalette( {
 		  )
 		: __( 'Custom color picker.' );
 
+	const paletteCommonProps = {
+		clearable,
+		clearColor,
+		onChange,
+		value,
+		actions: !! clearable && (
+			// @ts-ignore Required className property.
+			<CircularOptionPicker.ButtonAction onClick={ clearColor }>
+				{ __( 'Clear' ) }
+			</CircularOptionPicker.ButtonAction>
+		),
+	};
+
 	return (
 		<VStack spacing={ 3 } className={ className }>
 			{ ! disableCustomColors && (
@@ -295,24 +339,17 @@ export default function ColorPalette( {
 					) }
 				/>
 			) }
-			<Component
-				clearable={ clearable }
-				clearColor={ clearColor }
-				// @ts-ignore Component can be MultiplePalettes or SinglePalette, which have different colors prop types.
-				colors={ colors }
-				onChange={ onChange }
-				value={ value }
-				actions={
-					!! clearable && (
-						// @ts-ignore Required className property.
-						<CircularOptionPicker.ButtonAction
-							onClick={ clearColor }
-						>
-							{ __( 'Clear' ) }
-						</CircularOptionPicker.ButtonAction>
-					)
-				}
-			/>
+			{ __experimentalHasMultipleOrigins ? (
+				<MultiplePalettes
+					{ ...paletteCommonProps }
+					colors={ colors as MultipleColors[] }
+				/>
+			) : (
+				<SinglePalette
+					{ ...paletteCommonProps }
+					colors={ colors as Color[] }
+				/>
+			) }
 		</VStack>
 	);
 }
diff --git a/packages/components/src/color-palette/stories/index.tsx b/packages/components/src/color-palette/stories/index.tsx
index 4b88f093f7..559aadc783 100644
--- a/packages/components/src/color-palette/stories/index.tsx
+++ b/packages/components/src/color-palette/stories/index.tsx
@@ -51,7 +51,7 @@ export default meta;
 const Template: ComponentStory< typeof ColorPalette > = ( args ) => {
 	const firstColor =
 		( args.colors as Color[] )[ 0 ].color ||
-		( args.colors as MultipleColors )[ 0 ].colors[ 0 ].color;
+		( args.colors as MultipleColors[] )[ 0 ].colors[ 0 ].color;
 	const [ color, setColor ] = useState< string | undefined >( firstColor );
 
 	return (
diff --git a/packages/components/src/color-palette/types.ts b/packages/components/src/color-palette/types.ts
index 28169a939a..1376c1db38 100644
--- a/packages/components/src/color-palette/types.ts
+++ b/packages/components/src/color-palette/types.ts
@@ -1,19 +1,19 @@
 /**
  * External dependencies
  */
-import type { MouseEventHandler, ReactElement } from 'react';
+import type { CSSProperties, MouseEventHandler, ReactElement } from 'react';
 
 type OnColorChange = ( newColor?: string ) => void;
 
 export type Color = {
 	name: string;
-	color: string;
+	color: NonNullable< CSSProperties[ 'color' ] >;
 };
 
 export type MultipleColors = {
 	name: string;
 	colors: Color[];
-}[];
+};
 
 type PaletteProps = {
 	className?: string;
@@ -28,7 +28,7 @@ export type SinglePaletteProps = PaletteProps & {
 };
 
 export type MultiplePalettesProps = PaletteProps & {
-	colors: MultipleColors;
+	colors: MultipleColors[];
 };
 
 export type CustomColorPickerDropdownProps = {
@@ -52,8 +52,10 @@ export type ColorPaletteProps = {
 	className?: string;
 	/**
 	 * Array with the colors to be shown.
+	 *
+	 * @default []
 	 */
-	colors: MultipleColors | Color[];
+	colors?: ( MultipleColors | Color )[];
 	/**
 	 * Whether to allow custom color or not.
 	 */

packages/components/src/color-palette/types.ts Outdated Show resolved Hide resolved
packages/components/src/color-palette/types.ts Outdated Show resolved Hide resolved
packages/components/src/color-palette/index.tsx Outdated Show resolved Hide resolved
@kienstra
Copy link
Contributor Author

Hi @ciampo, thanks so much for reviewing this and for your suggestions! I'll apply them today.

Destructure props int constants after the function
signature, not inside it.
Types of property 'colors' are incompatible.
  Type 'Colors | undefined' is not assignable to type '(Color | MultipleColors)[]'.
    Type 'undefined' is not assignable to type '(Color | MultipleColors)[]'.
@kienstra
Copy link
Contributor Author

kienstra commented Oct 12, 2022

Out of curiosity — how did you come across the TypeScript migration?

Haha, I'm not sure.

I love TypeScript, and searched for TypeScript issues in Gutenberg 😄

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.

Just a few tweaks to the README!

- Type: `Boolean`
- Required: No
- Default: false

## Usage
Copy link
Contributor

Choose a reason for hiding this comment

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

As suggested in the guidelines, let's move the Usage section at the top of the document

diff --git a/packages/components/src/color-palette/README.md b/packages/components/src/color-palette/README.md
index 5d3ed4601c..49612e3f91 100644
--- a/packages/components/src/color-palette/README.md
+++ b/packages/components/src/color-palette/README.md
@@ -2,6 +2,36 @@
 
 `ColorPalette` allows the user to pick a color from a list of pre-defined color entries.
 
+## Usage
+
+```jsx
+import { ColorPalette } from '@wordpress/components';
+import { useState } from '@wordpress/element';
+
+const MyColorPalette = () => {
+	const [ color, setColor ] = useState ( '#f00' )
+	const colors = [
+		{ name: 'red', color: '#f00' },
+		{ name: 'white', color: '#fff' },
+		{ name: 'blue', color: '#00f' },
+	];
+
+	return (
+		<ColorPalette
+			colors={ colors }
+			value={ color }
+			onChange={ ( color ) => setColor( color ) }
+		/>
+	);
+} );
+```
+
+If you're using this component outside the editor, you can
+[ensure `Tooltip` positioning](/packages/components/README.md#popovers-and-tooltips)
+for the `ColorPalette`'s color swatches, by rendering your `ColorPalette` with a
+`Popover.Slot` further up the element tree and within a
+`SlotFillProvider` overall.
+
 ## Props
 
 The component accepts the following props.
@@ -75,33 +105,3 @@ Whether this is rendered in the sidebar.
 -   Type: `Boolean`
 -   Required: No
 -   Default: false
-
-## Usage
-
-```jsx
-import { ColorPalette } from '@wordpress/components';
-import { useState } from '@wordpress/element';
-
-const MyColorPalette = () => {
-	const [ color, setColor ] = useState ( '#f00' )
-	const colors = [
-		{ name: 'red', color: '#f00' },
-		{ name: 'white', color: '#fff' },
-		{ name: 'blue', color: '#00f' },
-	];
-
-	return (
-		<ColorPalette
-			colors={ colors }
-			value={ color }
-			onChange={ ( color ) => setColor( color ) }
-		/>
-	);
-} );
-```
-
-If you're using this component outside the editor, you can
-[ensure `Tooltip` positioning](/packages/components/README.md#popovers-and-tooltips)
-for the `ColorPalette`'s color swatches, by rendering your `ColorPalette` with a
-`Popover.Slot` further up the element tree and within a
-`SlotFillProvider` overall.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, committed in 4fd5d9c

packages/components/src/color-palette/README.md Outdated Show resolved Hide resolved
## Props

The component accepts the following props.

{ colors, disableCustomColors = false, value, onChange, className, clearable = true }
{ colors = [], disableCustomColors = false, enableAlpha, value, onchange, className, clearable = true, __experimentalHasMultipleOrigins = false, __experimentalIsRenderedInSidebar = false }
Copy link
Contributor

Choose a reason for hiding this comment

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

diff --git a/packages/components/src/color-palette/README.md b/packages/components/src/color-palette/README.md
index 49612e3f91..b194f3cef0 100644
--- a/packages/components/src/color-palette/README.md
+++ b/packages/components/src/color-palette/README.md
@@ -36,8 +36,6 @@ for the `ColorPalette`'s color swatches, by rendering your `ColorPalette` with a
 
 The component accepts the following props.
 
-{ colors = [],  disableCustomColors = false, enableAlpha, value, onchange, className, clearable = true, __experimentalHasMultipleOrigins = false, __experimentalIsRenderedInSidebar = false }
-
 ### colors
 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Committed in 29984c0

Comment on lines 11 to 16
### colors

Array with the colors to be shown.
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.

- Type: `Array`
- Required: Yes
- Required: No
- Default: `[]`
Copy link
Contributor

Choose a reason for hiding this comment

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

A bit nitty, but let's try to follow the format highlighted in the guidelines:

### `propName`: Typescript style type i.e `string`, `number`, `( nextValue: string ) => void`

Prop description. With a new line before and after the description and before and after type/required blocks.

-   Required: Either `Yes` or `No`
<!-- If the prop has a default value, add the following line: -->
-   Default: [default value]

For example, for the color prop the docs would change to

### `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.

-   Required: No
-   Default: `[]`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, good idea. Sorry for missing that in CONTRIBUTING.md.

Committed in 988e940

Comment on lines 63 to 78
### __experimentalHasMultipleOrigins

Whether the colors prop is an array of color palettes, rather than an array of color objects.

- Type: `Boolean`
- Required: No
- Default: false

### __experimentalIsRenderedInSidebar

Whether this is rendered in the sidebar.

- Type: `Boolean`
- Required: No
- Default: false

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's actually not list experimental props in the README — apologies if I forgot to mention that earlier!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, removed in 4fd5d9c

@kienstra
Copy link
Contributor Author

Thanks, good ideas! I'll apply these today.

@kienstra
Copy link
Contributor Author

Hi @ciampo,
Thanks a lot for your patience. The commits above apply your suggestions.

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.

Left a few minor comments, but once they are addressed we can merge this PR!

  • ✅ Code changes LGTM
  • ✅ Storybook examples work as expected
  • ✅ No regressions spotted in the editor

packages/components/src/color-palette/stories/index.tsx Outdated Show resolved Hide resolved
packages/components/src/color-palette/types.ts Outdated Show resolved Hide resolved
packages/components/src/color-palette/types.ts Outdated Show resolved Hide resolved
@kienstra
Copy link
Contributor Author

Thanks, Marco! Just committed your suggestions.

@ciampo ciampo merged commit dbbcc6e into WordPress:trunk Oct 26, 2022
@github-actions github-actions bot added this to the Gutenberg 14.5 milestone Oct 26, 2022
@kienstra kienstra deleted the update/color-palette-component-ts branch October 26, 2022 15:57
@kienstra
Copy link
Contributor Author

Hi Marco, thanks for your great ideas on the types and documentation here! And thanks for your patience.

@ciampo
Copy link
Contributor

ciampo commented Oct 26, 2022

Hi Marco, thanks for your great ideas on the types and documentation here! And thanks for your patience.

Thank you!

If you enjoyed this type of work, we have more components in need of a TypeScript refactor #35744

I'm sure that the next refactors will be quicker, as you get used to the migration requirements :)

@kienstra
Copy link
Contributor Author

Thanks, Marco! I'll open another PR to convert another component for #35744.

@Mamaduka Mamaduka added the Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release label Nov 10, 2022
Mamaduka pushed a commit that referenced this pull request Nov 10, 2022
* Convert the ColorPalette component to TypeScript

    There are some ts-ignore statements
    that would be good to remove.

* Correct the PR number in the CHANGELOG

* Apply Marco's suggestion to remove ts-ignore comments

Destructure props int constants after the function
signature, not inside it.

* Replace complex type with ReactNode, thanks to Marco's suggestion

* Apply Marco's suggestions for TS verbatim

#44632 (review)

* Prevent an error from colors possibly being undefined

Types of property 'colors' are incompatible.
  Type 'Colors | undefined' is not assignable to type '(Color | MultipleColors)[]'.
    Type 'undefined' is not assignable to type '(Color | MultipleColors)[]'.

* Rename Color and MultipleColors to ColorObject and PaletteObject

* Alphabetize the imports again

* Remove another needless ts-ignore comment

* Revert "Prevent an error from colors possibly being undefined"

This reverts commit 7fe648e.

* Make colors allow undefined

* Make actions optional, which I forgot before

* Commit Marco's changes, including a named export

* Add default tags, thanks to Marco's idea

* Apply Marco's suggestion to remove ts-ignore

Add 'as CSSProperties'
to remove the need for ts-ignore

* Apply Marco's suggestions, creating UnforwardedColorProps

The jsx example might not be right.
Also, I added a description for the component
in the JS DocBlock, as there wasn't one in README.md.
But maybe that's not right.

* Fix a linting error, remove needless className

* Commit Marco's suggestion: Update packages/components/src/color-palette/stories/index.tsx

Co-authored-by: Marco Ciampini <[email protected]>

* Commit Marco's suggestion: Update packages/components/src/color-palette/types.ts

Co-authored-by: Marco Ciampini <[email protected]>

* Rename test/index.js to test/indes.tsx, mv snapshot

* Add types to test/index.tsx

* Renamce styles.js to styles.ts

* Commit Marco's suggestion: Update packages/components/src/color-palette/types.ts

Co-authored-by: Marco Ciampini <[email protected]>

* Revert "Add types to test/index.tsx"

This reverts commit 06f7c4e.

* Paste Marco's description verbatim

#44632 (review)

* Copy props verbatim from types.ts into README.md

* Update the JSDoc description to be the same as the README.md description

The usage example was already the same
as in the README.md

* Remove extra entry for Tooltip

I think I added this
when merging in trunk and resolving the conflicts.

* Move the CHANGELOG entry up, to Unreleased

* Move Usage section to the top of the README. remove experimental props from README

* Commit Marco's suggestion: Update packages/components/src/color-palette/README.md

Co-authored-by: Marco Ciampini <[email protected]>

* Remove the example of the full props

* Change the props format to match CONTRIBUTING.md

https://github.com/WordPress/gutenberg/blob/a42805e157f6c6933f4ef7cabcfc87fa3af81aea/packages/components/CONTRIBUTING.md#readme-example

* Commit Marco's suggestion: Update packages/components/src/color-palette/stories/index.tsx

Co-authored-by: Marco Ciampini <[email protected]>

* Commit Marco's suggestion: Update packages/components/src/color-palette/types.ts

Co-authored-by: Marco Ciampini <[email protected]>

* Commit Marco's suggestion: Update packages/components/src/color-palette/types.ts

Co-authored-by: Marco Ciampini <[email protected]>

* Commit Marco's suggestion: Update packages/components/src/color-palette/types.ts

Co-authored-by: Marco Ciampini <[email protected]>

Co-authored-by: Marco Ciampini <[email protected]>
Mamaduka pushed a commit that referenced this pull request Nov 10, 2022
* Convert the ColorPalette component to TypeScript

    There are some ts-ignore statements
    that would be good to remove.

* Correct the PR number in the CHANGELOG

* Apply Marco's suggestion to remove ts-ignore comments

Destructure props int constants after the function
signature, not inside it.

* Replace complex type with ReactNode, thanks to Marco's suggestion

* Apply Marco's suggestions for TS verbatim

#44632 (review)

* Prevent an error from colors possibly being undefined

Types of property 'colors' are incompatible.
  Type 'Colors | undefined' is not assignable to type '(Color | MultipleColors)[]'.
    Type 'undefined' is not assignable to type '(Color | MultipleColors)[]'.

* Rename Color and MultipleColors to ColorObject and PaletteObject

* Alphabetize the imports again

* Remove another needless ts-ignore comment

* Revert "Prevent an error from colors possibly being undefined"

This reverts commit 7fe648e.

* Make colors allow undefined

* Make actions optional, which I forgot before

* Commit Marco's changes, including a named export

* Add default tags, thanks to Marco's idea

* Apply Marco's suggestion to remove ts-ignore

Add 'as CSSProperties'
to remove the need for ts-ignore

* Apply Marco's suggestions, creating UnforwardedColorProps

The jsx example might not be right.
Also, I added a description for the component
in the JS DocBlock, as there wasn't one in README.md.
But maybe that's not right.

* Fix a linting error, remove needless className

* Commit Marco's suggestion: Update packages/components/src/color-palette/stories/index.tsx

Co-authored-by: Marco Ciampini <[email protected]>

* Commit Marco's suggestion: Update packages/components/src/color-palette/types.ts

Co-authored-by: Marco Ciampini <[email protected]>

* Rename test/index.js to test/indes.tsx, mv snapshot

* Add types to test/index.tsx

* Renamce styles.js to styles.ts

* Commit Marco's suggestion: Update packages/components/src/color-palette/types.ts

Co-authored-by: Marco Ciampini <[email protected]>

* Revert "Add types to test/index.tsx"

This reverts commit 06f7c4e.

* Paste Marco's description verbatim

#44632 (review)

* Copy props verbatim from types.ts into README.md

* Update the JSDoc description to be the same as the README.md description

The usage example was already the same
as in the README.md

* Remove extra entry for Tooltip

I think I added this
when merging in trunk and resolving the conflicts.

* Move the CHANGELOG entry up, to Unreleased

* Move Usage section to the top of the README. remove experimental props from README

* Commit Marco's suggestion: Update packages/components/src/color-palette/README.md

Co-authored-by: Marco Ciampini <[email protected]>

* Remove the example of the full props

* Change the props format to match CONTRIBUTING.md

https://github.com/WordPress/gutenberg/blob/a42805e157f6c6933f4ef7cabcfc87fa3af81aea/packages/components/CONTRIBUTING.md#readme-example

* Commit Marco's suggestion: Update packages/components/src/color-palette/stories/index.tsx

Co-authored-by: Marco Ciampini <[email protected]>

* Commit Marco's suggestion: Update packages/components/src/color-palette/types.ts

Co-authored-by: Marco Ciampini <[email protected]>

* Commit Marco's suggestion: Update packages/components/src/color-palette/types.ts

Co-authored-by: Marco Ciampini <[email protected]>

* Commit Marco's suggestion: Update packages/components/src/color-palette/types.ts

Co-authored-by: Marco Ciampini <[email protected]>

Co-authored-by: Marco Ciampini <[email protected]>
@Mamaduka Mamaduka removed the Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release label Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] UI Components Impacts or related to the UI component system [Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

3 participants