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): unexpected props on a DOM element #2522

Merged
merged 18 commits into from
Apr 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
a2fc2bd
fix(popover): handle isDisabled logic for elements without isDisabled…
wingkwong Mar 14, 2024
47fc99b
chore(popover): isDisabled not necessary in restProps
wingkwong Mar 14, 2024
58ecceb
chore(changset): handle isDisabled logic for elements without isDisab…
wingkwong Mar 14, 2024
ac5cdda
fix(popover): keep all the props but isDisabled for non nextui button
wingkwong Mar 14, 2024
ccd4cfd
refactor(popover): move isDisabled handling to getTriggerProps
wingkwong Mar 15, 2024
730e499
refactor(popover): get the popover trigger styles from theme instead
wingkwong Mar 15, 2024
32340fd
feat(theme): add isDisabled styles in popover
wingkwong Mar 15, 2024
1001b5e
chore(changeset): add patch to @nextui-org/theme
wingkwong Mar 15, 2024
72b6fab
refactor(popover): avoid re-instantiate popover styles
wingkwong Mar 16, 2024
b976b09
fix(popover): apply filterDOMProps in popover trigger
wingkwong Mar 16, 2024
0147b9a
fix(popover): avoid conflicts with tooltip isDisabled
wingkwong Mar 16, 2024
7e8b4e8
chore(core): add isNextUIEl function to check if a component is a Nex…
jrgarciadev Mar 31, 2024
6023e89
chore(changeset): add system-rsc and revise message
wingkwong Mar 31, 2024
520615b
Merge branch 'main' into fix/eng-478
wingkwong Apr 1, 2024
f3c7c1d
feat(dropdown): add tests for custom trigger with isDisabled
wingkwong Apr 1, 2024
2188879
fix(dropdown): incorrect User import path
wingkwong Apr 1, 2024
1da2651
feat(dropdown): revise User and add mockRestore
wingkwong Apr 1, 2024
fc2c55a
fix(dropdown): revise user import path
wingkwong Apr 1, 2024
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
7 changes: 7 additions & 0 deletions .changeset/lovely-snakes-approve.md
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
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,
wingkwong marked this conversation as resolved.
Show resolved Hide resolved
}),
ref: mergeRefs(_ref, triggerRef),
};
},
Expand Down
9 changes: 9 additions & 0 deletions packages/core/system-rsc/src/utils.ts
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Loading