-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
GradientPicker: refactor to TypeScript #48316
Conversation
c43ea5c
to
3b8ccb4
Compare
3b8ccb4
to
82cc0fe
Compare
Size Change: +7 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
/** Start opting into the new margin-free styles that will become the default in a future version. */ | ||
__nextHasNoMargin = false, | ||
className, | ||
gradients, | ||
gradients = [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gradients
was previously listed as optional in the README file, but if it's omitted, the component breaks. Instead, we can default it to an empty array which shouldn't cause any problems.
8af1f9e
to
51085b7
Compare
51085b7
to
638f840
Compare
d446753
to
91eee49
Compare
Investigating test failures... |
Okay I see what's happening. After some testing, I think we should defer to @ciampo, I know we ran into a similar mismatch when reviewing the Proposed change:diff --git a/packages/components/src/palette-edit/types.ts b/packages/components/src/palette-edit/types.ts
index d5e0384c32..a918d1e874 100644
--- a/packages/components/src/palette-edit/types.ts
+++ b/packages/components/src/palette-edit/types.ts
@@ -7,6 +7,7 @@ import type { Key, MouseEventHandler } from 'react';
* Internal dependencies
*/
import type { HeadingSize } from '../heading/types';
+import type { GradientObject } from '../gradient-picker/types';
export type Color = {
color: string;
@@ -15,12 +16,7 @@ export type Color = {
gradient?: never;
};
-export type Gradient = {
- gradient?: string;
- name: string;
- slug: string;
- color?: never;
-};
+export type Gradient = GradientObject & { color?: never };
export type PaletteElement = Color | Gradient;
The If that works for you, I'm happy to include it in this PR, or as a separate PR that can be merged ahead of this one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we ran into a similar mismatch when reviewing the
CustomGradientPicker
changes, and avoided altering PaletteEdit, so I wanted your feedback before pushing changes you might want to roll back.
What part of the CustomGradientPicker
PR review are you referring to, in particular? I don't seem to be able to find that conversation
After some testing, I think we should defer to GradientPicker here. Passing a gradient object without a gradient string will result in a swatch that's either empty black or an ugly checkerboard, depending on selection state. Requiring the gradient value satisfies TypeScript without any side effects I can see.
This makes sense to me. A few reflections:
- would it be better, instead, to just make the
gradient
prop inPaletteEdit
required, instead of importingGradientPicker
's types? - alternatively, if we import
GradientPicker
's type for theGradient
object, should we also importColorPalette
's types for theColor
object?
That would be this comment, which you responded to inline recommending a change in
At first I was all for also importing from |
Thanks as always @ciampo. All current feedback should be addressed 👍 |
Flaky tests detected in 8b48b23. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4492358660
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
b2be4b1
to
8b48b23
Compare
What?
Refactor
GradientPicker
component to TypeScriptPart of #35744
Why?
The refactor to TypeScript has many benefits (auto-generated docs, static linting and error prevention, better IDE experience). See #35744 for more details
How?
Followed the steps in the TypeScript migration guide
Testing Instructions