Skip to content

Commit

Permalink
fix: on click event on interactive elements (#4322)
Browse files Browse the repository at this point in the history
  • Loading branch information
jrgarciadev authored Dec 10, 2024
1 parent a289ff8 commit 78c0928
Show file tree
Hide file tree
Showing 42 changed files with 366 additions and 1,233 deletions.
15 changes: 15 additions & 0 deletions .changeset/two-flies-design.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
---
"@nextui-org/use-aria-modal-overlay": patch
"@nextui-org/use-aria-button": patch
"@nextui-org/aria-utils": patch
"@nextui-org/dropdown": patch
"@nextui-org/use-aria-link": patch
"@nextui-org/listbox": patch
"@nextui-org/button": patch
"@nextui-org/navbar": patch
"@nextui-org/card": patch
"@nextui-org/link": patch
"@nextui-org/menu": patch
---

Fix #4292 interactive elements were not responding to "onClick" event
12 changes: 12 additions & 0 deletions packages/components/button/__tests__/button.test.tsx
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import "@testing-library/jest-dom";
import * as React from "react";
import {render} from "@testing-library/react";
import userEvent, {UserEvent} from "@testing-library/user-event";
Expand Down Expand Up @@ -35,6 +36,17 @@ describe("Button", () => {
expect(onPress).toHaveBeenCalled();
});

it("should trigger onClick function", async () => {
const onClick = jest.fn();
const {getByRole} = render(<Button disableRipple onClick={onClick} />);

const button = getByRole("button");

await user.click(button);

expect(onClick).toHaveBeenCalled();
});

it("should ignore events when disabled", async () => {
const onPress = jest.fn();
const {getByRole} = render(<Button disableRipple isDisabled onPress={onPress} />);
Expand Down
3 changes: 2 additions & 1 deletion packages/components/button/src/use-button.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ interface Props extends HTMLNextUIProps<"button"> {
/**
* The native button click event handler.
* use `onPress` instead.
* @deprecated
*/
onClick?: MouseEventHandler<HTMLButtonElement>;
}
Expand Down Expand Up @@ -150,7 +151,7 @@ export function useButton(props: UseButtonProps) {
elementType: as,
isDisabled,
onPress: chain(onPress, handlePress),
onClick: onClick,
onClick,
...otherProps,
} as AriaButtonProps,
domRef,
Expand Down
11 changes: 8 additions & 3 deletions packages/components/button/stories/button.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -71,14 +71,19 @@ const defaultProps = {
const StateTemplate = (args: ButtonProps) => {
const [isOpen, setIsOpen] = React.useState(false);

const handlePress = () => {
const handlePress = (e: any) => {
// eslint-disable-next-line no-console
console.log("Pressed");
console.log("Pressed", e);
setIsOpen((prev) => !prev);
};

return (
<Button {...args} aria-label="Open" aria-pressed={isOpen} onPress={handlePress}>
<Button
{...args}
aria-label={isOpen ? "Close" : "Open"}
aria-pressed={isOpen}
onClick={handlePress}
>
{isOpen ? "Close" : "Open"}
</Button>
);
Expand Down
21 changes: 18 additions & 3 deletions packages/components/card/__tests__/card.test.tsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,16 @@
import * as React from "react";
import {render} from "@testing-library/react";
import userEvent from "@testing-library/user-event";
import userEvent, {UserEvent} from "@testing-library/user-event";

import {Card} from "../src";

describe("Card", () => {
let user: UserEvent;

beforeEach(() => {
user = userEvent.setup();
});

it("should render correctly", () => {
const wrapper = render(<Card />);

Expand All @@ -30,13 +36,22 @@ describe("Card", () => {

const button = getByRole("button");

const user = userEvent.setup();

await user.click(button);

expect(onPress).toHaveBeenCalled();
});

it("should trigger onClick function", async () => {
const onClick = jest.fn();
const {getByRole} = render(<Card disableRipple isPressable onClick={onClick} />);

const button = getByRole("button");

await user.click(button);

expect(onClick).toHaveBeenCalled();
});

it("should render correctly when nested", () => {
const wrapper = render(
<Card>
Expand Down
11 changes: 8 additions & 3 deletions packages/components/card/src/use-card.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import type {AriaButtonProps} from "@nextui-org/use-aria-button";
import type {RippleProps} from "@nextui-org/ripple";

import {card} from "@nextui-org/theme";
import {ReactNode, useCallback, useMemo} from "react";
import {MouseEventHandler, ReactNode, useCallback, useMemo} from "react";
import {chain, mergeProps} from "@react-aria/utils";
import {useFocusRing} from "@react-aria/focus";
import {PressEvent, useHover} from "@react-aria/interactions";
Expand All @@ -20,7 +20,7 @@ import {ReactRef, filterDOMProps} from "@nextui-org/react-utils";
import {useDOMRef} from "@nextui-org/react-utils";
import {useRipple} from "@nextui-org/ripple";

export interface Props extends HTMLNextUIProps<"div"> {
export interface Props extends Omit<HTMLNextUIProps<"div">, "onClick"> {
/**
* Ref to the DOM node.
*/
Expand All @@ -34,12 +34,17 @@ export interface Props extends HTMLNextUIProps<"div"> {
* @default false
*/
disableRipple?: boolean;

/**
* Whether the card should allow text selection on press. (only for pressable cards)
* @default true
*/
allowTextSelectionOnPress?: boolean;
/**
* The native button click event handler.
* use `onPress` instead.
* @deprecated
*/
onClick?: MouseEventHandler<HTMLButtonElement>;
/**
* Classname or List of classes to change the classNames of the element.
* if `className` is passed, it will be added to the base slot.
Expand Down
30 changes: 30 additions & 0 deletions packages/components/card/stories/card.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,28 @@ const PrimaryActionTemplate = (args: CardProps) => {
);
};

const PressableTemplate = (args: CardProps) => {
// Both events should be fired when clicking on the card

const handlePress = () => {
// eslint-disable-next-line no-console
alert("card pressed");
};

const onClick = () => {
// eslint-disable-next-line no-console
alert("card clicked");
};

return (
<Card {...args} isPressable onClick={onClick} onPress={handlePress}>
<CardBody>
<p>Lorem ipsum dolor sit amet, consectetur adipiscing elit.</p>
</CardBody>
</Card>
);
};

const CenterImgWithHeaderTemplate = (args: CardProps) => {
const list = [
{
Expand Down Expand Up @@ -414,6 +436,14 @@ export const Default = {
},
};

export const Pressable = {
render: PressableTemplate,

args: {
...defaultProps,
},
};

export const WithDivider = {
render: WithDividerTemplate,

Expand Down
4 changes: 3 additions & 1 deletion packages/components/dropdown/stories/dropdown.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,9 @@ const Template = ({
<Button>{label}</Button>
</DropdownTrigger>
<DropdownMenu aria-label="Actions" color={color} variant={variant}>
<DropdownItem key="new">New file</DropdownItem>
<DropdownItem key="new" onClick={() => alert("New file")}>
New file
</DropdownItem>
<DropdownItem key="copy">Copy link</DropdownItem>
<DropdownItem key="edit">Edit file</DropdownItem>
<DropdownItem key="delete" className="text-danger" color="danger">
Expand Down
29 changes: 29 additions & 0 deletions packages/components/link/__tests__/link.test.tsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,16 @@
import * as React from "react";
import {render} from "@testing-library/react";
import userEvent, {UserEvent} from "@testing-library/user-event";

import {Link} from "../src";

describe("Link", () => {
let user: UserEvent;

beforeEach(() => {
user = userEvent.setup();
});

it("should render correctly", () => {
const wrapper = render(<Link />);

Expand Down Expand Up @@ -33,6 +40,28 @@ describe("Link", () => {
expect(container.querySelector("svg")).not.toBeNull();
});

it("should trigger onPress function", async () => {
const onPress = jest.fn();
const {getByRole} = render(<Link onPress={onPress} />);

const link = getByRole("link");

await user.click(link);

expect(onPress).toHaveBeenCalled();
});

it("should trigger onClick function", async () => {
const onClick = jest.fn();
const {getByRole} = render(<Link onClick={onClick} />);

const link = getByRole("link");

await user.click(link);

expect(onClick).toHaveBeenCalled();
});

it('should have target="_blank" and rel="noopener noreferrer" when "isExternal" is true', () => {
const {container} = render(
<Link isExternal href="#">
Expand Down
7 changes: 7 additions & 0 deletions packages/components/link/src/use-link.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import type {AriaLinkProps} from "@react-types/link";
import type {LinkVariantProps} from "@nextui-org/theme";
import type {MouseEventHandler} from "react";

import {link} from "@nextui-org/theme";
import {useAriaLink} from "@nextui-org/use-aria-link";
Expand Down Expand Up @@ -36,6 +37,12 @@ interface Props extends HTMLNextUIProps<"a">, LinkVariantProps {
* @default <LinkIcon />
*/
anchorIcon?: React.ReactNode;
/**
* The native link click event handler.
* use `onPress` instead.
* @deprecated
*/
onClick?: MouseEventHandler<HTMLAnchorElement>;
}

export type UseLinkProps = Props & AriaLinkProps;
Expand Down
29 changes: 28 additions & 1 deletion packages/components/link/stories/link.stories.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import type {VariantProps} from "@nextui-org/theme";

import {Meta} from "@storybook/react";
import React from "react";
import React, {useState} from "react";
import {tv} from "@nextui-org/theme";
import {link} from "@nextui-org/theme";

Expand Down Expand Up @@ -48,6 +48,22 @@ const defaultProps = {

const Template = (args: LinkProps) => <Link {...args} href="#" />;

const PressableTemplate = (args: LinkProps) => {
const [isOpen, setIsOpen] = useState(false);
const handlePress = (e: any) => {
// eslint-disable-next-line no-console
console.log("Pressed", e);

setIsOpen(!isOpen);
};

return (
<Link {...args} onClick={handlePress}>
{isOpen ? "Open" : "Close"}
</Link>
);
};

export const Default = {
render: Template,

Expand All @@ -59,6 +75,17 @@ export const Default = {
},
};

export const Pressable = {
render: PressableTemplate,

args: {
...defaultProps,
isDisabled: false,
color: "foreground",
size: "md",
},
};

export const Underline = Template.bind({}) as any;
Underline.args = {
...defaultProps,
Expand Down
11 changes: 9 additions & 2 deletions packages/components/listbox/src/base/listbox-item-base.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -89,11 +89,18 @@ interface Props<T extends object = {}> extends Omit<ItemProps<"li", T>, "childre
classNames?: SlotsToClasses<ListboxItemSlots>;
}

export type ListboxItemBaseProps<T extends object = {}> = Props<T> &
export type ListboxItemBaseProps<T extends object = {}> = Omit<Props<T>, "onClick"> &
Omit<ListboxItemVariantProps, "hasDescriptionTextChild" | "hasTitleTextChild"> &
Omit<AriaOptionProps, "key"> &
FocusableProps &
PressEvents;
PressEvents & {
/**
* The native click event handler.
* use `onPress` instead.
* @deprecated
*/
onClick?: (e: React.MouseEvent<HTMLLIElement | HTMLAnchorElement>) => void;
};

const ListboxItemBase = BaseItem as <T extends object>(
props: ListboxItemBaseProps<T>,
Expand Down
15 changes: 12 additions & 3 deletions packages/components/listbox/src/use-listbox-item.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import {
import {useFocusRing} from "@react-aria/focus";
import {Node} from "@react-types/shared";
import {filterDOMProps} from "@nextui-org/react-utils";
import {clsx, dataAttr, objectToDeps, removeEvents} from "@nextui-org/shared-utils";
import {clsx, dataAttr, objectToDeps, removeEvents, warn} from "@nextui-org/shared-utils";
import {useOption} from "@react-aria/listbox";
import {mergeProps} from "@react-aria/utils";
import {useHover, usePress} from "@react-aria/interactions";
Expand Down Expand Up @@ -46,7 +46,7 @@ export function useListboxItem<T extends object>(originalProps: UseListboxItemPr
classNames,
autoFocus,
onPress,
onClick,
onClick: deprecatedOnClick,
shouldHighlightOnFocus,
hideSelectedIcon = false,
isReadOnly = false,
Expand All @@ -68,6 +68,13 @@ export function useListboxItem<T extends object>(originalProps: UseListboxItemPr

const isMobile = useIsMobile();

if (deprecatedOnClick && typeof deprecatedOnClick === "function") {
warn(
"onClick is deprecated, please use onPress instead. See: https://github.com/nextui-org/nextui/issues/4292",
"ListboxItem",
);
}

const {pressProps, isPressed} = usePress({
ref: domRef,
isDisabled: isDisabled,
Expand Down Expand Up @@ -120,7 +127,9 @@ export function useListboxItem<T extends object>(originalProps: UseListboxItemPr
const getItemProps: PropGetter = (props = {}) => ({
ref: domRef,
...mergeProps(
{onClick},
{
onClick: deprecatedOnClick,
},
itemProps,
isReadOnly ? {} : mergeProps(focusProps, pressProps),
hoverProps,
Expand Down
Loading

0 comments on commit 78c0928

Please sign in to comment.