Skip to content

Commit

Permalink
fix(popover): unexpected props on a DOM element (#2522)
Browse files Browse the repository at this point in the history
* fix(popover): handle isDisabled logic for elements without isDisabled props

* chore(popover): isDisabled not necessary in restProps

* chore(changset): handle isDisabled logic for elements without isDisabled props

* fix(popover): keep all the props but isDisabled for non nextui button

* refactor(popover): move isDisabled handling to getTriggerProps

* refactor(popover): get the popover trigger styles from theme instead

* feat(theme): add isDisabled styles in popover

* chore(changeset): add patch to @nextui-org/theme

* refactor(popover): avoid re-instantiate popover styles

* fix(popover): apply filterDOMProps in popover trigger

* fix(popover): avoid conflicts with tooltip isDisabled

* chore(core): add isNextUIEl function to check if a component is a NextUI component

* chore(changeset): add system-rsc and revise message

* feat(dropdown): add tests for custom trigger with isDisabled

* fix(dropdown): incorrect User import path

* feat(dropdown): revise User and add mockRestore

* fix(dropdown): revise user import path

---------

Co-authored-by: Junior Garcia <jrgarciadev@gmail.com>
  • Loading branch information
wingkwong and jrgarciadev committed Apr 2, 2024
1 parent 410e30c commit c5049ed
Show file tree
Hide file tree
Showing 6 changed files with 92 additions and 5 deletions.
7 changes: 7 additions & 0 deletions .changeset/lovely-snakes-approve.md
@@ -0,0 +1,7 @@
---
"@nextui-org/popover": patch
"@nextui-org/system-rsc": patch
"@nextui-org/theme": patch
---

Fixed unexpected props on a DOM element (#2474)
45 changes: 45 additions & 0 deletions packages/components/dropdown/__tests__/dropdown.test.tsx
Expand Up @@ -2,6 +2,7 @@ import * as React from "react";
import {act, render} from "@testing-library/react";
import {Button} from "@nextui-org/button";
import userEvent from "@testing-library/user-event";
import {User} from "@nextui-org/user";

import {Dropdown, DropdownTrigger, DropdownMenu, DropdownItem, DropdownSection} from "../src";

Expand Down Expand Up @@ -416,4 +417,48 @@ describe("Dropdown", () => {

expect(onSelectionChange).toBeCalledTimes(0);
});

it("should render without error (custom trigger + isDisabled)", async () => {
const spy = jest.spyOn(console, "error").mockImplementation(() => {});

render(
<Dropdown isDisabled>
<DropdownTrigger>
<div>Trigger</div>
</DropdownTrigger>
<DropdownMenu aria-label="Actions" onAction={alert}>
<DropdownItem key="new">New file</DropdownItem>
<DropdownItem key="copy">Copy link</DropdownItem>
<DropdownItem key="edit">Edit file</DropdownItem>
<DropdownItem key="delete" color="danger">
Delete file
</DropdownItem>
</DropdownMenu>
</Dropdown>,
);

expect(spy).toBeCalledTimes(0);

spy.mockRestore();

render(
<Dropdown isDisabled>
<DropdownTrigger>
<User as="button" description="@tonyreichert" name="Tony Reichert" />
</DropdownTrigger>
<DropdownMenu aria-label="Actions" onAction={alert}>
<DropdownItem key="new">New file</DropdownItem>
<DropdownItem key="copy">Copy link</DropdownItem>
<DropdownItem key="edit">Edit file</DropdownItem>
<DropdownItem key="delete" color="danger">
Delete file
</DropdownItem>
</DropdownMenu>
</Dropdown>,
);

expect(spy).toBeCalledTimes(0);

spy.mockRestore();
});
});
23 changes: 19 additions & 4 deletions packages/components/popover/src/popover-trigger.tsx
@@ -1,6 +1,6 @@
import React, {Children, cloneElement, useMemo} from "react";
import {forwardRef} from "@nextui-org/system";
import {pickChildren} from "@nextui-org/react-utils";
import {forwardRef, isNextUIEl} from "@nextui-org/system";
import {pickChildren, filterDOMProps} from "@nextui-org/react-utils";
import {useAriaButton} from "@nextui-org/use-aria-button";
import {Button} from "@nextui-org/button";
import {mergeProps} from "@react-aria/utils";
Expand Down Expand Up @@ -29,7 +29,7 @@ const PopoverTrigger = forwardRef<"button", PopoverTriggerProps>((props, _) => {
};
}, [children]);

const {onPress, ...rest} = useMemo(() => {
const {onPress, ...restProps} = useMemo(() => {
return getTriggerProps(mergeProps(otherProps, child.props), child.ref);
}, [getTriggerProps, child.props, otherProps, child.ref]);

Expand All @@ -42,7 +42,22 @@ const PopoverTrigger = forwardRef<"button", PopoverTriggerProps>((props, _) => {
return triggerChildren?.[0] !== undefined;
}, [triggerChildren]);

return cloneElement(child, mergeProps(rest, hasNextUIButton ? {onPress} : buttonProps));
const isDisabled = !!restProps?.isDisabled;

const isNextUIElement = isNextUIEl(child);

return cloneElement(
child,
mergeProps(
// if we add `isDisabled` prop to DOM elements,
// react will fail to recognize it on a DOM element,
// hence, apply filterDOMProps for such case
filterDOMProps(restProps, {
enabled: isDisabled && !isNextUIElement,
}),
hasNextUIButton ? {onPress} : buttonProps,
),
);
});

PopoverTrigger.displayName = "NextUI.PopoverTrigger";
Expand Down
7 changes: 6 additions & 1 deletion packages/components/popover/src/use-popover.ts
Expand Up @@ -244,7 +244,12 @@ export function usePopover(originalProps: UsePopoverProps) {
"aria-haspopup": "dialog",
...mergeProps(triggerProps, props),
onPress,
className: slots.trigger({class: clsx(classNames?.trigger, props.className)}),
className: slots.trigger({
class: clsx(classNames?.trigger, props.className),
// apply isDisabled class names to make the trigger child disabled
// e.g. for elements like div or NextUI elements that don't have `isDisabled` prop
isDropdownDisabled: !!props?.isDisabled,
}),
ref: mergeRefs(_ref, triggerRef),
};
},
Expand Down
9 changes: 9 additions & 0 deletions packages/core/system-rsc/src/utils.ts
Expand Up @@ -100,3 +100,12 @@ export const mapPropsVariantsWithCommon = <
* Classnames utility
*/
export const cn = clsx;

/**
* Checks if a component is a NextUI component.
* @param component - The component to check.
* @returns `true` if the component is a NextUI component, `false` otherwise.
*/
export const isNextUIEl = (component: React.ReactComponentElement<any>) => {
return !!component.type?.render?.displayName?.includes("NextUI");
};
6 changes: 6 additions & 0 deletions packages/core/theme/src/components/popover.ts
Expand Up @@ -162,6 +162,12 @@ const popover = tv({
base: "animate-none",
},
},
isDropdownDisabled: {
true: {
trigger: "opacity-disabled pointer-events-none",
},
false: {},
},
},
defaultVariants: {
color: "default",
Expand Down

0 comments on commit c5049ed

Please sign in to comment.