Popover: refactor to TypeScript #43823
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
ciampo
commented
Sep 2, 2022
There was a problem hiding this comment.
Hey folks, I'm not completely done yet but this PR is already in a decent state where it wouldn't hurt if you could have a first look and start leaving feedback
cc @mirka @talldan @ntsekouras @walbo @aaronrobertshaw @andrewserong
| <SVG | ||
| { ...props } |
There was a problem hiding this comment.
Runtime change: the component was not receiving any props (and it's internal to this file)
Comment on lines
+127
to
+145
| const computedAnimationProps: HTMLMotionProps< 'div' > = | ||
| shouldAnimate && ! shouldReduceMotion | ||
| ? { | ||
| style: { | ||
| ...motionInlineStyles, | ||
| ...receivedInlineStyles, | ||
| }, | ||
| ...otherMotionProps, | ||
| onAnimationComplete, | ||
| animate: hasAnimatedOnce | ||
| ? false | ||
| : otherMotionProps.animate, | ||
| } | ||
| : { | ||
| animate: false, | ||
| style: receivedInlineStyles, | ||
| }; | ||
|
|
||
| return ( | ||
| <div | ||
| style={ receivedInlineStyles } | ||
| <motion.div | ||
| { ...computedAnimationProps } |
There was a problem hiding this comment.
Runtime change: always return a motion.div — I made this change because it would have been otherwise difficult to type AnimatedWrapper, since standard HTML elements and motion elements have different types for the same prop names
| let computedFlipProp = flip; | ||
| let computedResizeProp = resize; |
There was a problem hiding this comment.
Using new variables because flip and resize are readonly (const)
Comment on lines
+284
to
+304
| const { firstElementChild } = | ||
| refs.floating.current ?? {}; | ||
|
|
||
| // Only HTMLElement instances have the `style` property. | ||
| if ( ! ( firstElementChild instanceof HTMLElement ) ) | ||
| return; | ||
|
|
||
| // Reduce the height of the popover to the available space. | ||
| Object.assign( refs.floating.current.firstChild.style, { | ||
| maxHeight: `${ availableHeight }px`, | ||
| Object.assign( firstElementChild.style, { | ||
| maxHeight: `${ sizeProps.availableHeight }px`, |
There was a problem hiding this comment.
Runtime change: using firstElementChild instead of firstChild, since firstChild can return a Node (that doesn't have the style property). The additional instanceof HTMLElement makes TypeScript happy and adds an extra layer of precaution.
| @@ -270,14 +304,19 @@ const Popover = ( | |||
| } ) | |||
| : undefined, | |||
| arrow( { element: arrowRef } ), | |||
| ].filter( ( m ) => !! m ); | |||
| ].filter( | |||
| ( m: Middleware | undefined ): m is Middleware => m !== undefined | |||
There was a problem hiding this comment.
This was somehow necessary to make TypeScript happy
Comment on lines
+494
to
+515
| left: | ||
| typeof arrowData?.x !== 'undefined' && | ||
| Number.isFinite( arrowData.x ) | ||
| ? `${ | ||
| arrowData.x + | ||
| ( frameOffsetRef.current?.x ?? 0 ) | ||
| }px` | ||
| : '', | ||
| top: | ||
| typeof arrowData?.y !== 'undefined' && | ||
| Number.isFinite( arrowData.y ) | ||
| ? `${ | ||
| arrowData.y + | ||
| ( frameOffsetRef.current?.y ?? 0 ) | ||
| }px` | ||
| : '', |
There was a problem hiding this comment.
Runtime change: somehow Number.isFinite( arrowData.x ) was not guarding against arrowData.x not being undefined, and so I added an explicit check (same for arrowData.y)
Comment on lines
+546
to
+580
| // @ts-expect-error For Legacy Reasons | ||
| Popover.Slot = forwardRef( PopoverSlot ); | ||
| // @ts-expect-error For Legacy Reasons | ||
| Popover.__unstableSlotNameProvider = slotNameContext.Provider; | ||
|
|
||
| export default PopoverContainer; | ||
| export default Popover; |
There was a problem hiding this comment.
An alternative to this could be to:
- move the
Popovercomponent fromindex.tsxtocomponent.tsx - use
index.tsxto export theSlot,__unstableSlotNameProvideranddefaultexports — something like:
export function Slot (
{ name = SLOT_NAME }: { name?: string },
ref: ForwardedRef< any >
) {
return (
<Slot
// @ts-expect-error Need to type `SlotFill`
bubblesVirtually
name={ name }
className="popover-slot"
);
}
export const __unstableSlotNameProvider = slotNameContext.Provider;
export {Popover as default} from './component.tsx`;Thoughts? Could it work?
| /** @type {import('@floating-ui/react-dom').ReferenceType | undefined} */ | ||
| let referenceElement; | ||
| }: Pick< PopoverProps, 'anchorRef' | 'anchorRect' | 'getAnchorRect' > & { | ||
| fallbackReferenceElement: Element | null; |
There was a problem hiding this comment.
Runtime change: function return type changed to null instead of undefined, in order to match @floating-ui's setReference function signature (used in the Popover component)
| @@ -206,8 +228,9 @@ export const getReferenceElement = ( { | |||
| } else if ( fallbackReferenceElement ) { | |||
| // If no explicit ref is passed via props, fall back to | |||
| // anchoring to the popover's parent node. | |||
| referenceElement = fallbackReferenceElement.parentNode; | |||
| referenceElement = fallbackReferenceElement.parentElement; | |||
There was a problem hiding this comment.
Runtime change: using parentElement instead of parentNode, since Nodes don't have the getBoundingClientRect() function and therefore can't be used as the popover's reference (anchor)
This was referenced Sep 2, 2022
|
Size Change: +38 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
ciampo
commented
Sep 2, 2022
ciampo
added
[Type] Enhancement
ciampo
requested review from
mirka,
talldan,
ntsekouras,
walbo,
aaronrobertshaw and
andrewserong
ciampo
force-pushed
the
refactor/components-popover-typescript
branch
2 times, most recently
from
7ae4ba1
to
14b936a
Compare
ciampo
force-pushed
the
refactor/components-popover-typescript
branch
from
f973e49
to
b437435
Compare
There was a problem hiding this comment.
Great work as always @ciampo 👍
I've only had a chance to give this an initial read-through and test in Storybook, but so far it's testing well.
✅ No build or typing errors
✅ Unit tests pass for BorderControl, BorderBoxControl, and Popover.
✅ Didn't notice any inconsistent behaviour between this PR and trunk in Storybook. Tested Popover, border controls and others derived from the Popover e.g. Dropdown.
❓ I did spot a few typos and inconsistencies between the types.ts file and the Popover readme. I've left some inline comments for those.
I'll aim to do some more testing within the block and site editors in the next few days unless others beat me to it.
|
Thank you @aaronrobertshaw ! I missed your reviews :)
Thank you for pointing those out! The README should be in a much better place now
🙇 |
This was referenced Sep 6, 2022
mirka
approved these changes
Sep 6, 2022
There was a problem hiding this comment.
Nice! I did a smoke test of all the listed components in the editor, and nothing seemed off.
The only thing I noticed was that the color popover of PaletteEdit has a different offset compared to the color popovers in edit-post. (In edit-post, the popover doesn't overlap the writing flow scroll bar.) But this was not a regression in this PR, so 👍
| popoverProps={ { | ||
| ...__unstablePopoverProps, | ||
| } } | ||
| popoverProps={ __unstablePopoverProps } |
There was a problem hiding this comment.
Just checking: The reasoning behind this change is that consumers should be responsible for changing the object reference when necessary?
There was a problem hiding this comment.
No, I simply wanted to simplify the code and avoid the creation of a new object every time — was that intended? Happy to revert
There was a problem hiding this comment.
Seems intentional to me, although it's unclear whether it's warranted. If you haven't looked into it specifically, it might be better to bump that change to a separate issue, since it seems like the kind of thing that would have implications 👻 (It might be good implications though!)
I'll try to look into it after this PR is merged 👌 |
ciampo
force-pushed
the
refactor/components-popover-typescript
branch
from
57692b8
to
78a25e8
Compare
|
All feedback has been addressed — I will merge once CI is green ✅ |
14 tasks
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?
Part of #35744
Part of #42770
Refactor the
Popovercomponent to TypeScript.Unit tests will be converted separately, so that they can offer a better guarantee that the changes in this PR won't be introducing regressions.
Why?
See #35744
How?
Following the step-by-step guide in the CONTRIBUTING guidelines
Reviewing commit-by-commit may also offer an easier way to review this PR.
Testing Instructions
trunk