AnglePickerControl: refactor to TypeScript #45820
Conversation
ciampo
added
[Feature] UI Components
|
|
| if ( angleCircleRef.current === null ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Runtime change: added type guard, I think this represents as closely as possible the intentions of the original author
| const changeAngleToPosition = ( event ) => { | ||
| const { x: centerX, y: centerY } = angleCircleCenter.current; | ||
| const changeAngleToPosition = ( | ||
| event: ReactMouseEvent | MouseEvent | undefined |
There was a problem hiding this comment.
Type type is a bit cumbersome, but it is the union of all the different argument types that useDragging callbacks accept.
| if ( event === undefined ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Runtime change: type guard. It effectively doesn't make sense to run any code in this callback if the event object is not defined.
| if ( angleCircleCenter.current !== undefined ) { | ||
| const { x: centerX, y: centerY } = angleCircleCenter.current; | ||
| onChange( | ||
| getAngle( centerX, centerY, event.clientX, event.clientY ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Runtime change: to solve a TypeScript error, I moved the onChange( getAngle( ... ) ) call inside an angleCircleCenter.current !== undefined check. I thought this made sense, since getAngle requires both centerX and centerY to be valid numbers.
Alternatively, I could have defaulted centerX and centerY values to 0 when not defined, but I went for the solution that felt closer to the component's current behaviour.
| @@ -52,13 +79,12 @@ function AngleCircle( { value, onChange, ...props } ) { | |||
| } | |||
| document.body.style.cursor = 'grabbing'; | |||
| } else { | |||
| document.body.style.cursor = previousCursorValue.current || null; | |||
| document.body.style.cursor = previousCursorValue.current || ''; | |||
There was a problem hiding this comment.
Runtime change: the correct way to reset a CSS prop is via an empty string (TypeScript would otherwise complain)
| previousCursorValue.current = undefined; | ||
| } | ||
| }, [ isDragging ] ); | ||
|
|
||
| return ( | ||
| /* eslint-disable jsx-a11y/no-static-element-interactions */ |
There was a problem hiding this comment.
This disable comment wasn't necessary
| const inputValue = | ||
| unprocessedValue !== '' ? parseInt( unprocessedValue, 10 ) : 0; | ||
| unprocessedValue !== undefined && unprocessedValue !== '' |
There was a problem hiding this comment.
Runtime change: added an undefined type guard to fix a TypeScript error (previously the component would attempt to parseInt( undefined ), now it fallbacks to 0
| props: WordPressComponentProps< AnglePickerControlProps, 'div' >, | ||
| ref: ForwardedRef< any > |
There was a problem hiding this comment.
Runtime change: the component is now forwarding the remaining props to its outer div (including a ref)
| onChange, | ||
| ...args | ||
| } ) => { | ||
| const [ angle, setAngle ] = useState< number >( 0 ); |
There was a problem hiding this comment.
Notable change: the docs mentioned that both value and onChange are required props, but until this PR the Storybook example would have an initial state of undefined. I've fixed the issue by changing the initial value to 0, but we should make sure that the component is used as expected across the codebase.
|
Size Change: +42 B (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
| @@ -53,7 +53,7 @@ const GradientAnglePicker = ( { gradientAST, hasGradient, onChange } ) => { | |||
| <AnglePickerControl | |||
| __nextHasNoMarginBottom | |||
| onChange={ onAngleChange } | |||
| labelPosition="top" | |||
There was a problem hiding this comment.
This prop didn't seem to have any effects, judging by AngleControlPicker's code
| @@ -53,7 +53,7 @@ const GradientAnglePicker = ( { gradientAST, hasGradient, onChange } ) => { | |||
| <AnglePickerControl | |||
| __nextHasNoMarginBottom | |||
| onChange={ onAngleChange } | |||
| labelPosition="top" | |||
| // What should we pass here instead of `''` ? | |||
There was a problem hiding this comment.
As per comment — currently this usage of AngleControlPicker passes an empty string when the value is not defined, which is technically incorrect since value should have type number.
A few alternative approaches:
- Allow
valueto be of typestring | number - Allow
value(and thereforeonChange) to be optional, and therefore passundefinedinstead of'' - Use
0instead of''
What do folks think? My instinct tells me that allowing value and onChange to be optional would probably be the wisest change here, but I'm definitely open to more thoughts on the matter.
There was a problem hiding this comment.
I think making value, and onChange optional aligns more closely with the previous usage here though I don't hold a strong opinion either.
There was a problem hiding this comment.
I agree keeping it optional makes most sense from a developer's perspective 👍
There was a problem hiding this comment.
So! I went ahead and marked onChange and value as optional: 0451160
This in turn revealed another issue: with value and onChange as required, the component could only be used in controlled mode; marking those props as optional enables it to also be used in uncontrolled mode. This exposes a React error as soon we interact with the component in the GradientPicker, as we switch from an uncontrolled to controlled input.
Normally, a the way to solve this issue is to pass '' as the value, which initialises the component as controlled, while still not setting a value. But this is currently not compatible with AnglePickerControl's signature.
This "contrast" between the string and number types for the value prop is not new — in fact, we've been discussing it for a while, and it mostly has to do with NumberControl and it related component. And the more I think about it, the more I get the feeling that the string type is necessary for value, if anything to enable these components not to have an actual value while being controlled.
While the long term solution is clearly to make a decision about the type of the value prop in NumberControl and apply it to its derivate components (including AnglePickerControl), I've been thinking about the best compromise for this PR:
- we could revert
valueandonChangeto be required again, therefore avoid marking the component as usable in "uncontrolled" mode for now - we could change value's type to to
number | string, and therefore accept the''value thatCustomGradientPickeris passing
At a later point, we could:
- make the component usable in uncontrolled mode (i.e. marking
valueandonChangeas optional, and potentially use theuseControlledValuehook internally and make any further adjustment if needed) - adjust the type if needed once we make a decision on
NumberControl's type
Curious to hear y'all's thoughts!
There was a problem hiding this comment.
Thanks for the detailed summary of your findings so far. Life is rarely ever easy, is it 😁
If there is a particular option that reduces the potential for rework, I'd probably lean in that direction for now.
The same sort of logic would also suggest that thought and effort might be better directed to solving the problem at its source rather than agonising too much over which bandaid solution is best here.
There was a problem hiding this comment.
Life is rarely ever easy, is it 😁
It doesn't look like, does it? 😅
thought and effort might be better directed to solving the problem at its source rather
I think you're right, and I also think that working on this PR gave us enough insight on the fact that we sort of have to use string for the type, if anything in order to support properly controlled and uncontrolled version of a component.
I've opened #45949 to highlight a tentative plan of action and track progress on it. I will come back to this PR once I make the necessary pre-requisite changes
There was a problem hiding this comment.
Nice work @ciampo ✨
This tests well for me.
✅ No typing errors
✅ Unit tests for custom gradient picker pass
✅ Type descriptions and defaults match README
✅ AnglePickerControl, GradientPicker, and CustomGradientPicker work in Storybook
✅ Gradient pickers in the editor continue to function
Other than deciding on making value and onChange optional, this LGTM 👍
| @@ -53,7 +53,7 @@ const GradientAnglePicker = ( { gradientAST, hasGradient, onChange } ) => { | |||
| <AnglePickerControl | |||
| __nextHasNoMarginBottom | |||
| onChange={ onAngleChange } | |||
| labelPosition="top" | |||
| // What should we pass here instead of `''` ? | |||
There was a problem hiding this comment.
I think making value, and onChange optional aligns more closely with the previous usage here though I don't hold a strong opinion either.
| @@ -53,7 +53,7 @@ const GradientAnglePicker = ( { gradientAST, hasGradient, onChange } ) => { | |||
| <AnglePickerControl | |||
| __nextHasNoMarginBottom | |||
| onChange={ onAngleChange } | |||
| labelPosition="top" | |||
| // What should we pass here instead of `''` ? | |||
There was a problem hiding this comment.
I agree keeping it optional makes most sense from a developer's perspective 👍
ciampo
force-pushed
the
refator/components-angle-picker-control-typescript
branch
from
0451160 to
e8ef468
Compare
|
Flaky tests detected in a931eec. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4105493733
|
|
Since the work on In particular:
I also pushed some other minor refinements, like better What do y'all think about this approach? If you agree with it, could you give this PR a last round of reviews to make sure that Thank you! |
There was a problem hiding this comment.
What do y'all think about this approach?
The approach makes sense to me. Anything that doesn’t box us into a corner and unblocks further TypeScript migration work is a win.
✅ No typing errors
✅ Unit tests pass
✅ JSDocs & READMEs look good
✅ Impacted components all work in Storybook
✅ Gradient pickers in the editor work fine
LGTM 🚢
The failed e2e looks unrelated so I've kicked off re-running it.
ciampo
force-pushed
the
refator/components-angle-picker-control-typescript
branch
from
d2a0080 to
8f37200
Compare
ciampo
force-pushed
the
refator/components-angle-picker-control-typescript
branch
from
8f37200 to
a931eec
Compare
What?
Refactor the
AnglePickerControlcomponent 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 highlighted in the TypeScript migration guide.
Notable changes are flagged in the inline comments.
This PR may be more easily reviewed by looking at each commit separately
Testing Instructions
GradientPickerandCustomGradientPickerStorybook examples