-
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
Changes from all commits
525df66
b5e911a
db2ca4f
32c57cb
d98e61d
9896fe4
31b693c
87807c6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'@leafygreen-ui/hooks': minor | ||
--- | ||
|
||
Add `useMergeRefs` hook for merging array of refs into a single memoized callback ref or `null` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'@leafygreen-ui/popover': minor | ||
--- | ||
|
||
[LG-4515](https://jira.mongodb.org/browse/LG-4515) Replace internal position utils with [@floating-ui/react](https://floating-ui.com/docs/useFloating) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
import * as React from 'react'; | ||
|
||
/** | ||
* Merges an array of refs into a single memoized callback ref or `null`. | ||
*/ | ||
export function useMergeRefs<Instance>( | ||
refs: Array<React.Ref<Instance> | undefined>, | ||
): React.RefCallback<Instance> | null { | ||
return React.useMemo(() => { | ||
if (refs.every(ref => ref == null)) { | ||
return null; | ||
} | ||
|
||
return value => { | ||
refs.forEach(ref => { | ||
if (typeof ref === 'function') { | ||
ref(value); | ||
} else if (ref != null) { | ||
(ref as React.MutableRefObject<Instance | null>).current = value; | ||
} | ||
}); | ||
}; | ||
// eslint-disable-next-line react-hooks/exhaustive-deps | ||
}, refs); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,17 +22,18 @@ | |
"access": "public" | ||
}, | ||
"dependencies": { | ||
"@floating-ui/react": "^0.26.23", | ||
"@leafygreen-ui/emotion": "^4.0.8", | ||
"@leafygreen-ui/hooks": "^8.1.3", | ||
"@leafygreen-ui/lib": "^13.5.0", | ||
"@leafygreen-ui/portal": "^5.1.1", | ||
"@leafygreen-ui/tokens": "^2.8.0", | ||
"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" | ||
Comment on lines
-30
to
+32
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Attempted to replace |
||
}, | ||
"devDependencies": { | ||
"@leafygreen-ui/palette": "^4.0.9", | ||
"@leafygreen-ui/button": "^21.1.0", | ||
"@leafygreen-ui/palette": "^4.0.9", | ||
"@lg-tools/storybook-utils": "^0.1.0" | ||
}, | ||
"peerDependencies": { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,38 +2,27 @@ import React, { useMemo, useRef, useState } from 'react'; | |
|
||
import { | ||
useIsomorphicLayoutEffect, | ||
useMutationObserver, | ||
useObjectDependency, | ||
usePrevious, | ||
useViewportSize, | ||
} from '@leafygreen-ui/hooks'; | ||
|
||
import { getElementDocumentPosition } from './utils/positionUtils'; | ||
import { | ||
getElementDocumentPosition, | ||
getElementViewportPosition, | ||
} from './utils/positionUtils'; | ||
import { | ||
Align, | ||
Justify, | ||
PopoverProps, | ||
UseContentNodeReturnObj, | ||
UsePopoverPositioningProps, | ||
UseReferenceElementReturnObj, | ||
} from './Popover.types'; | ||
|
||
const mutationOptions = { | ||
// If attributes changes, such as className which affects layout | ||
attributes: true, | ||
// Watch if text changes in the node | ||
characterData: true, | ||
// Watch for any immediate children are modified | ||
childList: true, | ||
// Extend watching to entire sub tree to make sure we catch any modifications | ||
subtree: true, | ||
}; | ||
|
||
/** | ||
* This hook handles logic for determining the reference element for the popover element. | ||
* 1. If a `refEl` is provided, the ref value will be used as the reference element. | ||
* 2. If not, a hidden placeholder element will be rendered, and the parent element of the | ||
* placeholder will used as the reference element. | ||
* | ||
* Additionally, this hook calculates the document position of the reference element. | ||
*/ | ||
export function useReferenceElement( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
||
refEl?: PopoverProps['refEl'], | ||
scrollContainer?: PopoverProps['scrollContainer'], | ||
): UseReferenceElementReturnObj { | ||
const placeholderRef = useRef<HTMLSpanElement | null>(null); | ||
const [referenceElement, setReferenceElement] = useState<HTMLElement | null>( | ||
|
@@ -51,11 +40,19 @@ export function useReferenceElement( | |
if (maybeParentEl && maybeParentEl instanceof HTMLElement) { | ||
setReferenceElement(maybeParentEl); | ||
} | ||
}, [placeholderRef.current, refEl?.current]); | ||
}, [placeholderRef.current, refEl]); | ||
|
||
const referenceElDocumentPos = useObjectDependency( | ||
useMemo( | ||
() => getElementDocumentPosition(referenceElement, scrollContainer, true), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add TSDoc to |
||
[referenceElement, scrollContainer], | ||
), | ||
); | ||
|
||
return { | ||
placeholderRef, | ||
referenceElement, | ||
referenceElDocumentPos, | ||
renderHiddenPlaceholder: !refEl, | ||
}; | ||
} | ||
|
@@ -74,115 +71,3 @@ export function useContentNode(): UseContentNodeReturnObj { | |
setContentNode, | ||
}; | ||
} | ||
|
||
export function usePopoverPositioning({ | ||
active, | ||
adjustOnMutation, | ||
align = Align.Bottom, | ||
contentNode, | ||
justify = Justify.Start, | ||
referenceElement, | ||
scrollContainer, | ||
}: UsePopoverPositioningProps) { | ||
/** | ||
* Don't render the popover initially since computing the position depends on the window | ||
* which isn't available if the component is rendered on server side. | ||
*/ | ||
const [isReadyToRender, setIsReadyToRender] = useState(false); | ||
const [forceUpdateCounter, setForceUpdateCounter] = useState(0); | ||
|
||
/** | ||
* We calculate the position of the popover when it becomes active, so it's only safe | ||
* for us to enable the mutation observers once the popover is active. | ||
*/ | ||
const observeMutations = adjustOnMutation && active; | ||
|
||
const viewportSize = useViewportSize(); | ||
|
||
const lastTimeRefElMutated = useMutationObserver( | ||
referenceElement, | ||
mutationOptions, | ||
Date.now, | ||
observeMutations, | ||
); | ||
|
||
const lastTimeContentElMutated = useMutationObserver( | ||
contentNode?.parentNode as HTMLElement, | ||
mutationOptions, | ||
Date.now, | ||
observeMutations, | ||
); | ||
|
||
// We don't memoize these values as they're reliant on scroll positioning | ||
const referenceElViewportPos = useObjectDependency( | ||
getElementViewportPosition(referenceElement, scrollContainer, true), | ||
); | ||
|
||
// We use contentNode.parentNode since the parentNode has a transition applied to it and we want to be able to get the width of this element before it is transformed. Also as noted below, the parentNode cannot have a ref on it. | ||
// Previously the contentNode was passed in but since it is a child of transformed element it was not possible to get an untransformed width. | ||
const contentElViewportPos = useObjectDependency( | ||
getElementViewportPosition( | ||
contentNode?.parentNode as HTMLElement, | ||
scrollContainer, | ||
), | ||
); | ||
|
||
const referenceElDocumentPos = useObjectDependency( | ||
useMemo( | ||
() => getElementDocumentPosition(referenceElement, scrollContainer, true), | ||
[ | ||
referenceElement, | ||
scrollContainer, | ||
viewportSize, | ||
lastTimeRefElMutated, | ||
active, | ||
align, | ||
justify, | ||
forceUpdateCounter, | ||
], | ||
), | ||
); | ||
|
||
const contentElDocumentPos = useObjectDependency( | ||
useMemo( | ||
() => getElementDocumentPosition(contentNode), | ||
[ | ||
contentNode?.parentNode, | ||
viewportSize, | ||
lastTimeContentElMutated, | ||
active, | ||
align, | ||
justify, | ||
forceUpdateCounter, | ||
], | ||
), | ||
); | ||
|
||
const prevJustify = usePrevious<Justify>(justify); | ||
const prevAlign = usePrevious<Align>(align); | ||
|
||
const layoutMightHaveChanged = | ||
(prevJustify !== justify && | ||
(justify === Justify.Fit || prevJustify === Justify.Fit)) || | ||
(prevAlign !== align && justify === Justify.Fit); | ||
|
||
useIsomorphicLayoutEffect(() => { | ||
// justify={Justify.Fit} can cause the content's height/width to change | ||
// If we're switching to/from Fit, force an extra pass to make sure the popover is positioned correctly. | ||
// Also if we're switching between alignments and have Justify.Fit, it may switch between setting the width and | ||
// setting the height, so force an update in that case as well. | ||
if (layoutMightHaveChanged) { | ||
setForceUpdateCounter(n => n + 1); | ||
} | ||
}, [layoutMightHaveChanged]); | ||
|
||
useIsomorphicLayoutEffect(() => setIsReadyToRender(true), []); | ||
|
||
return { | ||
contentElDocumentPos, | ||
contentElViewportPos, | ||
isReadyToRender, | ||
referenceElDocumentPos, | ||
referenceElViewportPos, | ||
}; | ||
} |
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 useMergeRefsuseFloating
hook is shared from@floating-ui/react-dom
which is on a more robust and battle-tested v2.1.1useMergeRefs
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 layerThere 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
hookThere 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!