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

fix(popover): correct position logic #4498

Merged
merged 5 commits into from
Jan 5, 2025
Merged
Changes from 1 commit
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
Next Next commit
fix(popover): invalid placement logic
ryo-manba committed Jan 5, 2025
commit 6ddbc79c99178281782d8480d1f30024c4e21dcc
1 change: 0 additions & 1 deletion packages/components/dropdown/package.json
Original file line number Diff line number Diff line change
@@ -48,7 +48,6 @@
"@nextui-org/shared-utils": "workspace:*",
"@react-aria/focus": "3.19.0",
"@react-aria/menu": "3.16.0",
"@react-aria/overlays": "3.24.0",
"@react-aria/utils": "3.26.0",
"@react-stately/menu": "3.9.0",
"@react-types/menu": "3.9.13"
24 changes: 3 additions & 21 deletions packages/components/dropdown/src/use-dropdown.ts
Original file line number Diff line number Diff line change
@@ -9,12 +9,11 @@ import {useMenuTrigger} from "@react-aria/menu";
import {dropdown} from "@nextui-org/theme";
import {clsx} from "@nextui-org/shared-utils";
import {ReactRef, mergeRefs} from "@nextui-org/react-utils";
import {ariaShouldCloseOnInteractOutside, toReactAriaPlacement} from "@nextui-org/aria-utils";
import {ariaShouldCloseOnInteractOutside} from "@nextui-org/aria-utils";
import {useMemo, useRef} from "react";
import {mergeProps} from "@react-aria/utils";
import {MenuProps} from "@nextui-org/menu";
import {CollectionElement} from "@react-types/shared";
import {useOverlayPosition} from "@react-aria/overlays";

interface Props extends HTMLNextUIProps<"div"> {
/**
@@ -78,8 +77,6 @@ const getCloseOnSelect = <T extends object>(
return props?.closeOnSelect;
};

const DEFAULT_PLACEMENT = "bottom";

export function useDropdown(props: UseDropdownProps): UseDropdownReturn {
const globalContext = useProviderContext();

@@ -92,17 +89,13 @@ export function useDropdown(props: UseDropdownProps): UseDropdownReturn {
isDisabled,
type = "menu",
trigger = "press",
placement: placementProp = DEFAULT_PLACEMENT,
placement = "bottom",
closeOnSelect = true,
shouldBlockScroll = true,
classNames: classNamesProp,
disableAnimation = globalContext?.disableAnimation ?? false,
onClose,
className,
containerPadding = 12,
offset = 7,
crossOffset = 0,
shouldFlip = true,
...otherProps
} = props;

@@ -139,17 +132,6 @@ export function useDropdown(props: UseDropdownProps): UseDropdownReturn {
[className],
);

const {placement} = useOverlayPosition({
isOpen: state.isOpen,
targetRef: triggerRef,
overlayRef: popoverRef,
placement: toReactAriaPlacement(placementProp),
offset,
crossOffset,
shouldFlip,
containerPadding,
});

const onMenuAction = (menuCloseOnSelect?: boolean) => {
if (menuCloseOnSelect !== undefined && !menuCloseOnSelect) {
return;
@@ -164,7 +146,7 @@ export function useDropdown(props: UseDropdownProps): UseDropdownReturn {

return {
state,
placement: placement || DEFAULT_PLACEMENT,
placement,
ref: popoverRef,
disableAnimation,
shouldBlockScroll,
25 changes: 25 additions & 0 deletions packages/components/dropdown/stories/dropdown.stories.tsx
Original file line number Diff line number Diff line change
@@ -835,3 +835,28 @@ export const WithShouldBlockScroll = {
...defaultProps,
},
};

export const Placements = {
args: {
...defaultProps,
},
render: (args) => (
<div className="inline-grid grid-cols-3 gap-4">
<Template {...args} label="Top Start" placement="top-start" />
<Template {...args} label="Top" placement="top" />
<Template {...args} label="Top End" placement="top-end" />

<Template {...args} label="Bottom Start" placement="bottom-start" />
<Template {...args} label="Bottom" placement="bottom" />
<Template {...args} label="Bottom End" placement="bottom-end" />

<Template {...args} label="Right Start" placement="right-start" />
<Template {...args} label="Right" placement="right" />
<Template {...args} label="Right End" placement="right-end" />

<Template {...args} label="Left Start" placement="left-start" />
<Template {...args} label="Left" placement="left" />
<Template {...args} label="Left End" placement="left-end" />
</div>
),
};
2 changes: 1 addition & 1 deletion packages/components/popover/src/free-solo-popover.tsx
Original file line number Diff line number Diff line change
@@ -58,7 +58,7 @@ const FreeSoloPopoverWrapper = forwardRef<"div", FreeSoloPopoverWrapperProps>(
// @ts-ignore
transformOrigin,
};
} else {
} else if (placement) {
style = {
...style,
...getTransformOrigins(placement === "center" ? "top" : placement),
7 changes: 4 additions & 3 deletions packages/components/popover/src/popover-content.tsx
Original file line number Diff line number Diff line change
@@ -80,6 +80,9 @@ const PopoverContent = (props: PopoverContentProps) => {
);
}, [backdrop, disableAnimation, getBackdropProps]);

const style = placement
? getTransformOrigins(placement === "center" ? "top" : placement)
: undefined;
const contents = (
<>
{disableAnimation ? (
@@ -90,9 +93,7 @@ const PopoverContent = (props: PopoverContentProps) => {
animate="enter"
exit="exit"
initial="initial"
style={{
...getTransformOrigins(placement === "center" ? "top" : placement),
}}
style={style}
variants={TRANSITION_VARIANTS.scaleSpringOpacity}
{...motionProps}
>
25 changes: 20 additions & 5 deletions packages/components/popover/src/use-popover.ts
Original file line number Diff line number Diff line change
@@ -8,6 +8,7 @@ import {OverlayTriggerState, useOverlayTriggerState} from "@react-stately/overla
import {useFocusRing} from "@react-aria/focus";
import {ariaHideOutside, useOverlayTrigger, usePreventScroll} from "@react-aria/overlays";
import {OverlayTriggerProps} from "@react-types/overlays";
import {getShouldUseAxisPlacement} from "@nextui-org/aria-utils";
import {
HTMLNextUIProps,
mapPropsVariants,
@@ -152,7 +153,11 @@ export function usePopover(originalProps: UsePopoverProps) {

const state = stateProp || innerState;

const {popoverProps, underlayProps, placement} = useReactAriaPopover(
const {
popoverProps,
underlayProps,
placement: ariaPlacement,
} = useReactAriaPopover(
{
triggerRef,
isNonModal,
@@ -174,6 +179,16 @@ export function usePopover(originalProps: UsePopoverProps) {
state,
);

const placement = useMemo(() => {
// If ariaPlacement is null, popoverProps.style isn't set,
// so we return null to avoid an incorrect animation value.
if (!ariaPlacement) {
return null;
}

return getShouldUseAxisPlacement(ariaPlacement, placementProp) ? ariaPlacement : placementProp;
}, [ariaPlacement, placementProp]);
Comment on lines +182 to +190
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Incorrect initial placement caused animation to be applied in the wrong direction.


const {triggerProps} = useOverlayTrigger({type: triggerType}, state, triggerRef);

const {isFocusVisible, isFocused, focusProps} = useFocusRing();
@@ -206,7 +221,7 @@ export function usePopover(originalProps: UsePopoverProps) {
"data-focus": dataAttr(isFocused),
"data-arrow": dataAttr(showArrow),
"data-focus-visible": dataAttr(isFocusVisible),
"data-placement": getArrowPlacement(placement || DEFAULT_PLACEMENT, placementProp),
"data-placement": ariaPlacement ? getArrowPlacement(ariaPlacement, placementProp) : undefined,
...mergeProps(focusProps, dialogPropsProp, props),
className: slots.base({class: clsx(baseStyles)}),
style: {
@@ -220,10 +235,10 @@ export function usePopover(originalProps: UsePopoverProps) {
"data-slot": "content",
"data-open": dataAttr(state.isOpen),
"data-arrow": dataAttr(showArrow),
"data-placement": getArrowPlacement(placement || DEFAULT_PLACEMENT, placementProp),
"data-placement": ariaPlacement ? getArrowPlacement(ariaPlacement, placementProp) : undefined,
className: slots.content({class: clsx(classNames?.content, props.className)}),
}),
[slots, state.isOpen, showArrow, placement, placementProp, classNames],
[slots, state.isOpen, showArrow, placement, placementProp, classNames, ariaPlacement],
);

const onPress = useCallback(
@@ -307,7 +322,7 @@ export function usePopover(originalProps: UsePopoverProps) {
classNames,
showArrow,
triggerRef,
placement: placement || DEFAULT_PLACEMENT,
placement,
isNonModal,
popoverRef: domRef,
portalContainer,
119 changes: 58 additions & 61 deletions pnpm-lock.yaml

Large diffs are not rendered by default.