-
Notifications
You must be signed in to change notification settings - Fork 63
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
LG-4515: replace position utils with @floating-ui/react #2473
Conversation
🦋 Changeset detectedLatest commit: 87807c6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Size Change: -2.86 kB (-0.21%) Total Size: 1.36 MB
ℹ️ View Unchanged
|
897771e
to
13898e7
Compare
13898e7
to
421d274
Compare
421d274
to
9896fe4
Compare
@@ -22,17 +22,18 @@ | |||
"access": "public" | |||
}, | |||
"dependencies": { | |||
"@floating-ui/react": "^0.26.23", |
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.
Note on pre-v1 dependency usage
The 2 things we are importing from @floating-ui/react
are useFloating and useMergeRefs
- The
useFloating
hook is shared from@floating-ui/react-dom
which is on a more robust and battle-tested v2.1.1 - The
useMergeRefs
hook is a low-level hook and also is the only reason (currently) that I'm using@floating-ui/react
rather than@floating-ui/react-dom
. May use more from this package when adding support for the top layer
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.
TBH we should probably have our own useMergeRefs
hook
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.
added!
"react-transition-group": "^4.4.5", | ||
"@types/react-transition-group": "^4.4.5" | ||
"@types/react-transition-group": "^4.4.5", | ||
"react-transition-group": "^4.4.5" |
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.
Attempted to replace react-transition-group
usage with @floating-ui/react transition hooks, but this unfortunately proved to be a larger lift due to consuming packages that rely on the react-transition-group
lifecycle method API. May revisit this at a later time but opting to leave as-is for now to focus on other priorities
storyNames: [ | ||
'Top', | ||
'Right', | ||
'Bottom', | ||
'Left', | ||
'CenterHorizontal', | ||
'CenterVertical', | ||
], |
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.
broke up the generated stories because there was impact to positioning for popovers that were not initially in the viewport
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.
Noticed we're still disabling chromatic as well. If there are still issues with flakiness, I was able to somewhat solve this for the CN menus with an async play function:
Story.play = async ({ canvasElement }) => {
// (optional) if using a trigger
const trigger = await within(canvasElement).findByTestId(...);
userEvent.click(trigger);
// delays snapshot until the element is in the DOM
await within(document.body).findByTestId(...);
}
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.
@TheSonOfThomp can you clarify what you're referring to with "still disabling chromatic"? I'm seeing it's only disabled for the LiveExample
and ScrollableContainer
instances
opacity: 1; | ||
position: ${usePortal ? '' : 'absolute'}; |
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.
removed this because it always gets set to position: absolute;
regardless
{ | ||
[css` | ||
transition-delay: 0ms; | ||
`]: state === 'exiting', | ||
}, |
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.
this doesn't seem to be necessary after migrating to Floating UI
@@ -53,9 +34,17 @@ export function useReferenceElement( | |||
} | |||
}, [placeholderRef.current, refEl?.current]); |
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 think the deps array should just be refEl
here?
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.
without including placeholderRef.current
, the referenceElement
doesn't get properly get set
I think refEl
works fine so I can update that
// Extend watching to entire sub tree to make sure we catch any modifications | ||
subtree: true, | ||
}; | ||
|
||
export function useReferenceElement( |
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.
Can we add TSDoc for what this hook does/how it should be used?
@@ -53,9 +34,17 @@ export function useReferenceElement( | |||
} | |||
}, [placeholderRef.current, refEl?.current]); | |||
|
|||
const referenceElDocumentPos = useObjectDependency( | |||
useMemo( | |||
() => getElementDocumentPosition(referenceElement, scrollContainer, true), |
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.
Can we add TSDoc to getElementDocumentPosition
as well?
* In addition to these placements, we override the `align` prop when it is set to 'center-horizontal' or | ||
* 'center-vertical' to create custom placements {@see https://floating-ui.com/docs/offset#creating-custom-placements} | ||
*/ | ||
export const getFloatingPlacement = ( |
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.
So this function converts our Align
& Justify
enums into a placement value that useFloating
accepts?
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.
yes, I updated the tsdocs description to make this more explicit
<Instance refEl={ref} /> | ||
<Button> | ||
Button Text | ||
<Instance /> |
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.
why did we move the Instance
inside the button? Is refEl
not the recommended way to do this?
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 did that to get rid of the eslint-disable-next-line
for the useRef
so there's less to read / make sense of
AFAIK there is no recommended approach between providing or not providing refEl
export const Top = () => {}; | ||
Top.args = { | ||
align: Align.Top, | ||
}; |
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.
super nit, but I'm prefering StoryObj
notation for stories like this
export const Top = {
render: () => <></>,
args: {
align: Align.Top
}
}
({ rects }) => { | ||
if (align === Align.CenterHorizontal) { | ||
return -rects.reference.width / 2 - rects.floating.width / 2; | ||
} | ||
|
||
if (align === Align.CenterVertical) { | ||
return -rects.reference.height / 2 - rects.floating.height / 2; | ||
} | ||
|
||
return spacing; |
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.
Could we abstract this fn somewhere?
export const CenterHorizontal = () => {}; | ||
CenterHorizontal.args = { | ||
align: Align.CenterHorizontal, | ||
}; | ||
|
||
export const CenterVertical = () => {}; | ||
CenterVertical.args = { | ||
align: Align.CenterVertical, | ||
}; |
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.
@@ -50,6 +51,12 @@ type Justify = (typeof Justify)[keyof typeof Justify]; | |||
|
|||
export { Justify }; | |||
|
|||
export type ExtendedPlacement = | |||
| Placement |
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.
Are we still supporting Justify.Fit
? Should that be marked @deprecated
?
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.
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 initially included the deprecation in this PR but I instead broke it off into this PR: #2474
@@ -188,25 +195,25 @@ export const ScrollableContainer: StoryFn<PopoverStoryProps> = ({ | |||
}: PopoverStoryProps) => { | |||
const [active, setActive] = useState<boolean>(false); | |||
const portalRef = useRef<HTMLElement | null>(null); | |||
const portalContainer = useRef<HTMLDivElement | null>(null); | |||
const scrollContainer = useRef<HTMLDivElement | null>(null); | |||
|
|||
const position = referenceElPositions[refButtonPosition]; |
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.
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.
moving it how so? there's some additional repositioning that happens if adjustOnMutation
is turned on but what were you specifically wondering about?
I know you haven't changed it in this PR, but should the default spacing be 8px/ |
probably can reduce this default to 4 or 6 px. confirming with design on this edit: added the change here #2479 |
* Add @floating-ui/react dependency * Clean up Popover stories * Replace position utils and usePopoverPositioning with useFloating * Remove replaced positioning hook and utils * Fix adjustOnMutation and stories * Changeset * Add useMergeRefs hook * Address feedback
* Add @floating-ui/react dependency * Clean up Popover stories * Replace position utils and usePopoverPositioning with useFloating * Remove replaced positioning hook and utils * Fix adjustOnMutation and stories * Changeset * Add useMergeRefs hook * Address feedback
* Add @floating-ui/react dependency * Clean up Popover stories * Replace position utils and usePopoverPositioning with useFloating * Remove replaced positioning hook and utils * Fix adjustOnMutation and stories * Changeset * Add useMergeRefs hook * Address feedback
✍️ Proposed changes
@floating-ui/react
🎟 Jira ticket: LG-4515
✅ Checklist
For bug fixes, new features & breaking changes
yarn changeset
and documented my changes🧪 How to test changes
Popover
stories