Skip to content
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-4447: refactor Popover positioning logic and remove unused contentClassName prop #2457

Merged
merged 8 commits into from
Sep 3, 2024
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/loud-ears-share.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@leafygreen-ui/popover': major
---

[LG-4447](https://jira.mongodb.org/browse/LG-4447) Remove unused `contentClassName` prop and abstract popover positioning logic into custom hooks
5 changes: 5 additions & 0 deletions .changeset/tame-islands-count.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@leafygreen-ui/date-picker': patch
---

Remove unused popover `contentClassName` prop
1 change: 0 additions & 1 deletion packages/date-picker/src/DatePicker/DatePicker.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3708,7 +3708,6 @@ describe('packages/date-picker', () => {
onExit={() => {}}
onExiting={() => {}}
onExited={() => {}}
contentClassName=""
/>
</>;
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ export const modifiedPopoverPropNames: Array<ModifiedPopoverPropkeys> = [
'onExit',
'onExiting',
'onExited',
'contentClassName',
];

/**
Expand Down
28 changes: 28 additions & 0 deletions packages/popover/src/Popover.components.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import React, { forwardRef, ReactNode } from 'react';

import { contentClassName, hiddenPlaceholderStyle } from './Popover.styles';

export const HiddenPlaceholder = forwardRef<HTMLSpanElement, {}>(
(_, fwdRef) => {
/**
* Using \<span\> as placeholder to prevent validateDOMNesting warnings
* Warnings will still show up if `usePortal` is false
*/
return <span ref={fwdRef} className={hiddenPlaceholderStyle} />;
},
);

HiddenPlaceholder.displayName = 'HiddenPlaceholder';

export const ContentWrapper = forwardRef<
HTMLDivElement,
{ children: ReactNode }
>(({ children }, fwdRef) => {
return (
<div ref={fwdRef} className={contentClassName}>
{children}
</div>
);
});

ContentWrapper.displayName = 'ContentWrapper';
191 changes: 191 additions & 0 deletions packages/popover/src/Popover.hooks.tsx
shaneeza marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed this before, but we usually don't create a single file for hooks and components. We make an individual file for each component, and we add hooks to the utils folder. To be consistent with the other components, I think it makes sense to use the same pattern. Check out Toast as an example.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see polymorphic does a *.hooks.ts file too. I think explicitly separating hooks from utils is more conventional and similarly supported with how we have a hooks package. I will leave this as-is for now but removed the components and reverted to the previous JSX. Also will add this as a discussion topic for the next dev sync

Original file line number Diff line number Diff line change
@@ -0,0 +1,191 @@
import React, { useMemo, useRef, useState } from 'react';

import {
useIsomorphicLayoutEffect,
useMutationObserver,
useObjectDependency,
usePrevious,
useViewportSize,
} from '@leafygreen-ui/hooks';

import {
getElementDocumentPosition,
getElementViewportPosition,
} from './utils/positionUtils';
import { ContentWrapper, HiddenPlaceholder } from './Popover.components';
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,
};

export function useReferenceElement(
refEl?: PopoverProps['refEl'],
): UseReferenceElementReturnObj {
const placeholderRef = useRef<HTMLSpanElement | null>(null);
const [referenceElement, setReferenceElement] = useState<HTMLElement | null>(
null,
);

useIsomorphicLayoutEffect(() => {
if (refEl && refEl.current) {
setReferenceElement(refEl.current);
}

const placeholderEl = placeholderRef?.current;
const maybeParentEl = placeholderEl !== null && placeholderEl?.parentNode;

if (maybeParentEl && maybeParentEl instanceof HTMLElement) {
setReferenceElement(maybeParentEl);
}
}, [placeholderRef.current, refEl?.current]);

return {
HiddenPlaceholder,
placeholderRef,
referenceElement,
renderHiddenPlaceholder: !refEl,
};
}

export function useContentNode(): UseContentNodeReturnObj {
const [contentNode, setContentNode] = React.useState<HTMLDivElement | null>(
null,
);

const contentNodeRef = useRef(contentNode);
contentNodeRef.current = contentNode;

return {
contentNode,
contentNodeRef,
ContentWrapper,
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,
};
}
70 changes: 70 additions & 0 deletions packages/popover/src/Popover.styles.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
import { TransitionStatus } from 'react-transition-group';

import { css, cx } from '@leafygreen-ui/emotion';
import { createUniqueClassName } from '@leafygreen-ui/lib';
import { transitionDuration } from '@leafygreen-ui/tokens';

import { AbsolutePositionObject } from './utils/positionUtils';

export const TRANSITION_DURATION = transitionDuration.default;

export const contentClassName = createUniqueClassName('popover-content');

export const hiddenPlaceholderStyle = css`
display: none;
`;

type PositionCSS = AbsolutePositionObject & { transformOrigin: string };

const getBasePopoverStyles = (positionCSS: PositionCSS) => css`
position: absolute;
top: ${positionCSS.top};
left: ${positionCSS.left};
right: ${positionCSS.right};
bottom: ${positionCSS.bottom};
transform-origin: ${positionCSS.transformOrigin};
transition-property: opacity, transform;
transition-duration: ${TRANSITION_DURATION}ms;
transition-timing-function: ease-in-out;
opacity: 0;
`;

const getActiveStyles = (usePortal: boolean) => css`
opacity: 1;
position: ${usePortal ? '' : 'absolute'};
pointer-events: initial;
`;

export const getPopoverStyles = ({
className,
popoverZIndex,
positionCSS,
state,
transform,
usePortal,
}: {
className?: string;
popoverZIndex?: number;
positionCSS: PositionCSS;
state: TransitionStatus;
transform: string;
usePortal: boolean;
}) =>
cx(
getBasePopoverStyles(positionCSS),
{
[css`
transform: ${transform};
`]: state === 'entering' || state === 'exiting',
[getActiveStyles(usePortal)]: state === 'entered',
[css`
z-index: ${popoverZIndex};
`]: typeof popoverZIndex === 'number',
},
className,
{
[css`
transition-delay: 0ms;
`]: state === 'exiting',
},
);
Loading
Loading