CustomGradientPicker: Refactor to TypeScript #48929
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +655 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
chad1008
commented
Mar 8, 2023
packages/components/src/custom-gradient-picker/gradient-bar/index.tsx
Outdated
Show resolved
Hide resolved
chad1008
commented
Mar 8, 2023
chad1008
commented
Mar 8, 2023
chad1008
commented
Mar 8, 2023
chad1008
commented
Mar 8, 2023
chad1008
commented
Mar 8, 2023
chad1008
commented
Mar 8, 2023
chad1008
force-pushed
the
refactor/CustomGradientPicker-typescript
branch
from
5b0663c to
58661fd
Compare
chad1008
added
[Type] Code Quality
chad1008
commented
Mar 8, 2023
ciampo
reviewed
Mar 8, 2023
packages/components/src/custom-gradient-picker/gradient-bar/control-points.tsx
Outdated
Show resolved
Hide resolved
ciampo
reviewed
Mar 8, 2023
packages/components/src/custom-gradient-picker/gradient-bar/control-points.tsx
Outdated
Show resolved
Hide resolved
ciampo
reviewed
Mar 8, 2023
ciampo
reviewed
Mar 8, 2023
packages/components/src/custom-gradient-picker/gradient-bar/control-points.tsx
Outdated
Show resolved
Hide resolved
ciampo
reviewed
Mar 8, 2023
packages/components/src/custom-gradient-picker/gradient-bar/control-points.tsx
Show resolved
Hide resolved
ciampo
reviewed
Mar 8, 2023
ciampo
reviewed
Mar 9, 2023
packages/components/src/custom-gradient-picker/gradient-bar/control-points.tsx
Outdated
Show resolved
Hide resolved
ciampo
reviewed
Mar 9, 2023
packages/components/src/custom-gradient-picker/gradient-bar/utils.ts
Outdated
Show resolved
Hide resolved
packages/components/src/custom-gradient-picker/gradient-bar/utils.ts
Outdated
Show resolved
Hide resolved
packages/components/src/custom-gradient-picker/gradient-bar/utils.ts
Outdated
Show resolved
Hide resolved
packages/components/src/custom-gradient-picker/stories/index.tsx
Outdated
Show resolved
Hide resolved
chad1008
force-pushed
the
refactor/CustomGradientPicker-typescript
branch
2 times, most recently
from
56b15f2 to
a33606e
Compare
|
Whew, thanks for all of the detailed feedback, as always @ciampo. I think I've addressed everything. There are a couple of unresolved conversations in the thread I had questions or needed feedback on. Beyond that, the conflicts listed are updating the CHANGELOG and addressing some added |
…lPoints` declaration
chad1008
force-pushed
the
refactor/CustomGradientPicker-typescript
branch
from
21dcf47 to
7595d97
Compare
|
Rebased and incorporated changes. I did need to make one small change (7595d97) to the new types in |
ciampo
reviewed
Mar 16, 2023
packages/components/src/custom-gradient-picker/gradient-bar/utils.ts
Outdated
Show resolved
Hide resolved
Comment on lines
181
to
184
| export function getHorizontalRelativeGradientPosition( | ||
| mouseXcoordinate: number, | ||
| containerElement: HTMLDivElement | null | ||
| ): number | undefined; |
There was a problem hiding this comment.
I think that your initial implementation was correct — we don't really need to advertise this type for the getHorizontalRelativeGradientPosition function
Suggested change
| export function getHorizontalRelativeGradientPosition( | |
| mouseXcoordinate: number, | |
| containerElement: HTMLDivElement | null | |
| ): number | undefined; |
There was a problem hiding this comment.
This comment probably got lost in the mountain of threads we've accumulated, but I think we do actually need the union override.
If you don't get the same error, or if there's a better solution, I'm happy to investigate further!
There was a problem hiding this comment.
I definitely missed it 😓
Anyway, I took a look and I see why we'd need the third overload. Although I also looked at the TS error, and I think that a better solution would be to tweak the early return check in the GradientBar, which would then allows us to remove that third function overload:
diff --git a/packages/components/src/custom-gradient-picker/gradient-bar/index.tsx b/packages/components/src/custom-gradient-picker/gradient-bar/index.tsx
index 526ee53bff..225a5fb757 100644
--- a/packages/components/src/custom-gradient-picker/gradient-bar/index.tsx
+++ b/packages/components/src/custom-gradient-picker/gradient-bar/index.tsx
@@ -96,15 +96,15 @@ export default function CustomGradientBar( {
const onMouseEnterAndMove: MouseEventHandler< HTMLDivElement > = (
event
) => {
+ if ( ! gradientMarkersContainerDomRef.current ) {
+ return;
+ }
+
const insertPosition = getHorizontalRelativeGradientPosition(
event.clientX,
gradientMarkersContainerDomRef.current
);
- if ( insertPosition === undefined ) {
- return;
- }
-
// If the insert point is close to an existing control point don't show it.
if (
controlPoints.some( ( { position } ) => {
diff --git a/packages/components/src/custom-gradient-picker/gradient-bar/utils.ts b/packages/components/src/custom-gradient-picker/gradient-bar/utils.ts
index 6e9417328c..88daf2b2c1 100644
--- a/packages/components/src/custom-gradient-picker/gradient-bar/utils.ts
+++ b/packages/components/src/custom-gradient-picker/gradient-bar/utils.ts
@@ -178,10 +178,6 @@ export function getHorizontalRelativeGradientPosition(
mouseXcoordinate: number,
containerElement: null
): undefined;
-export function getHorizontalRelativeGradientPosition(
- mouseXcoordinate: number,
- containerElement: HTMLDivElement | null
-): number | undefined;
export function getHorizontalRelativeGradientPosition(
mouseXCoordinate: number,
containerElement: HTMLDivElement | nullWhat do you think?
There was a problem hiding this comment.
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
…l with `false` default
|
Flaky tests detected in 465a59d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4448835977
|
… to better align with `ColorPickerPopover` types
|
Applied most recent feedback. I believe there is only one remaining open item. I did introduce one additional small change. tl;dr: |
ciampo
approved these changes
Mar 17, 2023
There was a problem hiding this comment.
This was a tough one! Thank you @chad1008 for the patience in addressing all the feedback. I think we did a good job and introduced very few runtime changes 🚀
Good to merge once this last comment is addressed
|
Last change implemented, waiting on CI before merging... |
This was referenced Mar 20, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Refactor
CustomGradientPickercomponent 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
npm ci, as a newDefinitelyTypedpackage has been added for thegradientParserlibrary this component relies on.Notes:
DefinitelyTypedvalues for thegradientParserlibrary. This led to what felt like pretty liberal use of type assertions. Many of these were to avoid conflicts between union members. Anyway, I know there's a bunch of them and I'm very open to feedback them... so anywhere you see me using anas, please take it with a grain of salt and don't hesitate to redirect me if you see a better option.