-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
fix: on click event on interactive elements #4322
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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 |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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} | ||||||||||||||||||||||||||
| > | ||||||||||||||||||||||||||
|
Comment on lines
+81
to
+86
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Update to use onPress instead of onClick Since <Button
{...args}
aria-label={isOpen ? "Close" : "Open"}
aria-pressed={isOpen}
- onClick={handlePress}
+ onPress={handlePress}
>📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||
| {isOpen ? "Close" : "Open"} | ||||||||||||||||||||||||||
| </Button> | ||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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> | ||||||||||||||
|
Comment on lines
+151
to
+153
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider removing the onClick handler in favor of onAction The component already has an
Remove the onClick handler since the action is already handled by onAction: - <DropdownItem key="new" onClick={() => alert("New file")}>
+ <DropdownItem key="new">
New file
</DropdownItem>📝 Committable suggestion
Suggested change
|
||||||||||||||
| <DropdownItem key="copy">Copy link</DropdownItem> | ||||||||||||||
| <DropdownItem key="edit">Edit file</DropdownItem> | ||||||||||||||
| <DropdownItem key="delete" className="text-danger" color="danger"> | ||||||||||||||
|
|
||||||||||||||
| 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"; | ||
|
|
||
|
|
@@ -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); | ||
| }; | ||
|
Comment on lines
+53
to
+58
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Improve type safety and use onPress instead of onClick The current implementation has several issues:
Consider this improvement: - const handlePress = (e: any) => {
- // eslint-disable-next-line no-console
- console.log("Pressed", e);
- setIsOpen(!isOpen);
- };
+ const handlePress = (e: PressEvent) => {
+ console.log("Pressed", e);
+ setIsOpen(!isOpen);
+ };And update the Link usage: - <Link {...args} onClick={handlePress}>
+ <Link {...args} onPress={handlePress}>
|
||
|
|
||
| return ( | ||
| <Link {...args} onClick={handlePress}> | ||
| {isOpen ? "Open" : "Close"} | ||
| </Link> | ||
| ); | ||
| }; | ||
|
|
||
| export const Default = { | ||
| render: Template, | ||
|
|
||
|
|
@@ -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, | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -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"> & | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Replace empty object type with a more specific type Using -export type ListboxItemBaseProps<T extends object = {}> = Omit<Props<T>, "onClick"> &
+export type ListboxItemBaseProps<T extends Record<string, unknown> = Record<string, unknown>> = Omit<Props<T>, "onClick"> &📝 Committable suggestion
Suggested change
🧰 Tools🪛 Biome (1.9.4)[error] 92-92: Don't use '{}' as a type. Prefer explicitly define the object shape. '{}' means "any non-nullable value". (lint/complexity/noBannedTypes) |
||||||
| 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>, | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Replace
anytype with specific event typeUsing
anytype reduces type safety. Since this is a button click event, we should use the specific event type.📝 Committable suggestion