fix: on click event on interactive elements #4322
Conversation
🦋 Changeset detectedLatest commit: ca3f51a The changes in this PR will be included in the next version bump. This PR includes changesets to release 25 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request introduces a series of modifications across various components in the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (15)
packages/components/navbar/package.json (1)
51-51: Good architectural choice moving to @react-aria/buttonThe switch from an internal toggle button implementation to React Aria's specialized button package is a solid improvement. React Aria provides battle-tested accessibility features and robust interaction handling.
packages/components/menu/src/base/menu-item-base.tsx (1)
96-102: Enhance the deprecation notice with more detailsThe deprecation notice is good, but could be more helpful to users by including:
- The rationale for switching to
onPress- The version when
onClickwill be removed- A migration example
/** * The native click event handler. - * use `onPress` instead. + * @deprecated Since v2.x.x. Use `onPress` instead for better accessibility and cross-platform compatibility. + * Migration: Replace `onClick={handler}` with `onPress={handler}`. + * Will be removed in v3.0.0. */ onClick?: MouseEventHandler<HTMLLIElement | HTMLAnchorElement>;packages/hooks/use-aria-modal-overlay/src/index.ts (2)
15-22: Enhance JSDoc documentation with parameters and return value.The current documentation clearly explains the purpose and modifications. Consider enhancing it with:
@paramdocumentation forprops,state, andrefparameters@returnsdocumentation describing theModalOverlayAriastructure@exampleshowing basic usageHere's a suggested enhancement:
/** * Provides the behavior and accessibility implementation for a modal component. * A modal is an overlay element which blocks interaction with elements outside it. * * This is a modified version from https://vscode.dev/github/adobe/react-spectrum/blob/main/packages/%40react-aria/overlays/src/useModalOverlay.ts#L46 * * This implementation disables the prevent scroll when `shouldBlockScroll` prop is false. + * + * @param props - The props for the modal overlay + * @param props.shouldBlockScroll - Whether to prevent scrolling when modal is open + * @param state - The overlay trigger state + * @param ref - The ref object for the modal element + * @returns Object containing props for the modal and underlay elements + * + * @example + * ```tsx + * function Modal() { + * const ref = useRef(null); + * const state = useOverlayTriggerState({}); + * const { modalProps, underlayProps } = useAriaModalOverlay({}, state, ref); + * + * return ( + * <div {...underlayProps}> + * <div {...modalProps} ref={ref}> + * Modal content + * </div> + * </div> + * ); + * } + * ``` */
Line range hint
23-28: Consider using a more idiomatic TypeScript default props pattern.The current default props implementation could be improved for better type inference and maintainability.
Here's a suggested refactoring:
-export function useAriaModalOverlay( - props: UseAriaModalOverlayProps & { - shouldBlockScroll?: boolean; - } = { - shouldBlockScroll: true, - }, +interface UseAriaModalOverlayProps extends AriaModalOverlayProps { + shouldBlockScroll?: boolean; +} + +const defaultProps: Partial<UseAriaModalOverlayProps> = { + shouldBlockScroll: true, +}; + +export function useAriaModalOverlay( + props: UseAriaModalOverlayProps = defaultProps, state: OverlayTriggerState, ref: RefObject<HTMLElement>, ): ModalOverlayAria {packages/hooks/use-aria-button/src/index.ts (1)
105-106: Consider Enhancing Mobile Detection LogicCurrently,
isMobileis determined usingisIOS()orisAndroid(). To ensure broader compatibility, consider incorporating additional checks for other mobile platforms or using a more general mobile detection approach..changeset/two-flies-design.md (1)
15-15: Enhance Changeset Message for ClarityThe changeset message provides a basic summary of the fix. Consider adding more details to clarify the changes, such as mentioning the deprecation of
onClickin favor ofonPress, to inform users about the impact on the API.packages/components/card/__tests__/card.test.tsx (1)
44-53: Add deprecation notice in test descriptionSince
onClickis being deprecated in favor ofonPress, consider updating the test description to document this:- it("should trigger onClick function", async () => { + it("should trigger onClick function (deprecated: use onPress instead)", async () => {packages/components/link/__tests__/link.test.tsx (1)
43-63: Consider combining event handler tests
- Add deprecation notice for onClick
- Consider combining the tests to reduce duplication:
- 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 trigger event handlers", async () => { + const onPress = jest.fn(); + const onClick = jest.fn(); // deprecated: use onPress instead + const {getByRole} = render(<Link onPress={onPress} onClick={onClick} />); + + const link = getByRole("link"); + await user.click(link); + + expect(onPress).toHaveBeenCalled(); + expect(onClick).toHaveBeenCalled(); + });packages/components/listbox/src/base/listbox-item-base.tsx (1)
97-102: Enhance deprecation notice documentationThe deprecation notice for
onClickis good, but could be more helpful by including:
- The version where it will be removed
- A code example showing migration to
onPress/** * The native click event handler. * use `onPress` instead. + * @example + * ```tsx + * // Before + * <ListboxItem onClick={(e) => {}} /> + * + * // After + * <ListboxItem onPress={(e) => {}} /> + * ``` + * @deprecated Since v2.0.0. Will be removed in v3.0.0 */packages/components/link/src/use-link.ts (1)
Line range hint
83-83: Consider improving type safetyThe ts-ignore comment suggests a type mismatch between useLink and useAriaLink.
Consider updating the types in useAriaLink to properly handle the onClick prop, eliminating the need for ts-ignore.
packages/components/listbox/src/use-listbox-item.ts (1)
130-132: Consider handling the event type conversionThe deprecated onClick is passed directly to mergeProps. Consider adding type conversion similar to the MenuItem implementation to ensure type safety.
{ - onClick: deprecatedOnClick, + onClick: (e: React.MouseEvent<HTMLLIElement>) => + deprecatedOnClick?.(e), },packages/components/button/src/use-button.ts (1)
154-155: Consider adding the deprecation warningWhile the deprecation is documented, consider adding the runtime warning similar to other components for consistency.
{ + onClick: (e) => { + if (onClick) { + warn( + "onClick is deprecated, please use onPress instead. See: https://github.com/nextui-org/nextui/issues/4292", + "Button" + ); + onClick(e); + } + }, ...otherProps, }packages/components/card/src/use-card.ts (1)
42-47: Consider enhancing the deprecation notice.While the onClick property is correctly marked as deprecated, the documentation could be more helpful by providing a migration example.
Consider updating the JSDoc comment to include a migration example:
/** * The native button click event handler. * use `onPress` instead. * @deprecated + * @example + * ```tsx + * // Before + * <Card onClick={(e) => console.log(e)}> + * // After + * <Card onPress={() => console.log('pressed')}> + * ``` */packages/components/menu/stories/menu.stories.tsx (1)
48-50: Consider using console.log instead of alert in stories.While the onClick handler demonstrates the functionality, using alert in stories can be disruptive during development. Consider using console.log for a better developer experience.
- <MenuItem key="new" onClick={() => alert("[onClick] New file")}> + <MenuItem key="new" onClick={() => console.log("[onClick] New file")}>packages/components/card/stories/card.stories.tsx (1)
351-371: Improve the story implementation for better developer experience.The template demonstrates both onClick and onPress handlers, but there are a few improvements that could be made:
- Using alerts can be disruptive during development
- The comment could be more specific about the expected behavior
Consider this implementation:
const PressableTemplate = (args: CardProps) => { - // Both events should be fired when clicking on the card + // This example demonstrates the coexistence of both onClick and onPress handlers. + // Note: onClick is deprecated and will be removed in a future version. const handlePress = () => { - // eslint-disable-next-line no-console - alert("card pressed"); + console.log("onPress event fired"); }; const onClick = () => { - // eslint-disable-next-line no-console - alert("card clicked"); + console.log("onClick event fired (deprecated)"); };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (41)
.changeset/two-flies-design.md(1 hunks)packages/components/button/__tests__/button.test.tsx(2 hunks)packages/components/button/src/use-button.ts(2 hunks)packages/components/button/stories/button.stories.tsx(1 hunks)packages/components/card/__tests__/card.test.tsx(2 hunks)packages/components/card/src/use-card.ts(3 hunks)packages/components/card/stories/card.stories.tsx(2 hunks)packages/components/dropdown/stories/dropdown.stories.tsx(1 hunks)packages/components/link/__tests__/link.test.tsx(2 hunks)packages/components/link/src/use-link.ts(2 hunks)packages/components/link/stories/link.stories.tsx(3 hunks)packages/components/listbox/src/base/listbox-item-base.tsx(1 hunks)packages/components/listbox/src/use-listbox-item.ts(4 hunks)packages/components/listbox/stories/listbox.stories.tsx(1 hunks)packages/components/menu/__tests__/menu.test.tsx(3 hunks)packages/components/menu/package.json(0 hunks)packages/components/menu/src/base/menu-item-base.tsx(2 hunks)packages/components/menu/src/menu.tsx(0 hunks)packages/components/menu/src/use-menu-item.ts(5 hunks)packages/components/menu/src/use-menu.ts(1 hunks)packages/components/menu/stories/menu.stories.tsx(1 hunks)packages/components/navbar/package.json(1 hunks)packages/components/navbar/src/navbar-menu-toggle.tsx(1 hunks)packages/hooks/use-aria-button/package.json(1 hunks)packages/hooks/use-aria-button/src/index.ts(4 hunks)packages/hooks/use-aria-link/package.json(1 hunks)packages/hooks/use-aria-link/src/index.ts(3 hunks)packages/hooks/use-aria-menu/CHANGELOG.md(0 hunks)packages/hooks/use-aria-menu/README.md(0 hunks)packages/hooks/use-aria-menu/package.json(0 hunks)packages/hooks/use-aria-menu/src/index.ts(0 hunks)packages/hooks/use-aria-menu/src/use-menu-item.ts(0 hunks)packages/hooks/use-aria-menu/src/use-menu.ts(0 hunks)packages/hooks/use-aria-menu/tsconfig.json(0 hunks)packages/hooks/use-aria-modal-overlay/src/index.ts(1 hunks)packages/hooks/use-aria-toggle-button/CHANGELOG.md(0 hunks)packages/hooks/use-aria-toggle-button/README.md(0 hunks)packages/hooks/use-aria-toggle-button/package.json(0 hunks)packages/hooks/use-aria-toggle-button/src/index.ts(0 hunks)packages/hooks/use-aria-toggle-button/tsconfig.json(0 hunks)packages/utilities/aria-utils/src/overlays/ariaHideOutside.ts(2 hunks)
💤 Files with no reviewable changes (14)
- packages/hooks/use-aria-menu/README.md
- packages/hooks/use-aria-toggle-button/tsconfig.json
- packages/hooks/use-aria-toggle-button/README.md
- packages/hooks/use-aria-menu/tsconfig.json
- packages/hooks/use-aria-toggle-button/package.json
- packages/components/menu/package.json
- packages/hooks/use-aria-menu/package.json
- packages/hooks/use-aria-toggle-button/CHANGELOG.md
- packages/hooks/use-aria-menu/CHANGELOG.md
- packages/components/menu/src/menu.tsx
- packages/hooks/use-aria-toggle-button/src/index.ts
- packages/hooks/use-aria-menu/src/index.ts
- packages/hooks/use-aria-menu/src/use-menu.ts
- packages/hooks/use-aria-menu/src/use-menu-item.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/components/listbox/src/base/listbox-item-base.tsx
[error] 92-92: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
packages/components/menu/src/base/menu-item-base.tsx
[error] 91-91: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🔇 Additional comments (30)
packages/components/menu/src/use-menu.ts (1)
132-132: LGTM: Proper event handler propagation
The change correctly forwards the onAction handler to the underlying aria menu implementation, ensuring proper event handling while maintaining accessibility.
packages/components/menu/__tests__/menu.test.tsx (3)
350-352: LGTM: Valid test for MenuItem classNames
The test correctly verifies that custom classNames are properly applied to MenuItem components.
363-365: LGTM: Proper override behavior test
The test ensures that individual MenuItem classNames correctly override menu-level itemClasses, maintaining proper styling precedence.
375-378: LGTM: Comprehensive class merging test
The test thoroughly validates that:
- Individual MenuItem classNames are merged with menu-level itemClasses
- Default itemClasses are applied when no override is present
packages/components/navbar/package.json (1)
51-51: Verify @react-aria/button version selection
Let's ensure version 3.11.0 is appropriate and check for any known issues.
✅ Verification successful
@react-aria/button version 3.11.0 is appropriate
The version 3.11.0 is currently the latest version available on npm, and there are no known security vulnerabilities reported. The recent releases of the React Spectrum ecosystem (which includes React Aria) don't mention any critical updates or breaking changes for the button component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check latest versions and changelogs for @react-aria/button
# Get latest version from npm
echo "Latest version from npm:"
npm view @react-aria/button version
# Check for security advisories
echo -e "\nSecurity advisories:"
npm audit @react-aria/button@3.11.0 --json | jq '.metadata.vulnerabilities'
# Get changelog entries since 3.11.0
echo -e "\nChangelog entries since 3.11.0:"
gh api graphql -f query='
{
repository(owner: "adobe", name: "react-spectrum") {
releases(first: 5, orderBy: {field: CREATED_AT, direction: DESC}) {
nodes {
tagName
description
}
}
}
}'
Length of output: 10153
packages/utilities/aria-utils/src/overlays/ariaHideOutside.ts (2)
9-14: Great type-safety improvements!
The addition of the ObserverWrapper interface and the explicit typing of observerStack enhances code maintainability and prevents potential type-related issues.
44-46: Verify behavior with orphaned nodes
While the null check for node.parentElement improves safety, please verify that skipping the role check for orphaned nodes (nodes without parents) doesn't introduce inconsistencies in the accessibility tree.
Let's check for any existing tests or usages that might be affected:
✅ Verification successful
The parent element check is correctly implemented
The code modification is safe and intentional. The search results reveal that this check is specifically designed to handle VoiceOver iOS issues with role="row" elements (as commented in the code). The parent element null check is a proper safety measure, and skipping orphaned nodes is the correct behavior because:
- The code is part of the
ariaHideOutsideutility which specifically deals with hiding elements in relation to their parent-child hierarchy - The special handling for
role="row"is a targeted fix for a WebKit bug (#222623) affecting VoiceOver on iOS - Orphaned nodes (without parents) cannot be part of a table row structure, so the
role="row"check is irrelevant for them
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test cases involving orphaned nodes or role="row"
rg -A 5 "role=\"row\"" --type=test
rg -A 5 "removeChild|parentElement" --type=test
# Search for related accessibility tests
rg -A 5 "aria-hidden.*row" --type=test
Length of output: 212
Script:
#!/bin/bash
# Search for test cases involving orphaned nodes or role="row"
rg -A 5 'role="row"'
rg -A 5 'removeChild|parentElement'
# Search for related accessibility tests
rg -A 5 'aria-hidden.*row'
# Look for test files specifically
fd -e test.ts -e spec.ts -e test.tsx -e spec.tsx
Length of output: 21414
packages/hooks/use-aria-modal-overlay/src/index.ts (1)
19-21: Verify consistent usage of the modified modal hook across the codebase.
Since this is a modified version of Adobe's implementation with different scroll prevention behavior, we should ensure consistent usage across all modal components.
Let's verify the hook usage:
✅ Verification successful
Let me gather more information about modal-related components and their implementation.
Modified modal hook is being used consistently across the codebase
The verification shows that:
- The custom
useAriaModalOverlayhook is only imported and used in the main Modal component (packages/components/modal/src/use-modal.ts) - There are no direct imports of Adobe's original
useModalOverlay - The
shouldBlockScrollprop is consistently implemented across related components (Modal, Popover, Navbar, Dropdown) - The default behavior (shouldBlockScroll=true) aligns with the original Adobe implementation while providing the flexibility to disable it when needed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any direct imports of Adobe's useModalOverlay
rg "from ['|\"]@react-aria/overlays['|\"].*useModalOverlay"
# Search for all uses of our useAriaModalOverlay hook
rg "useAriaModalOverlay"
# Search for modal components that might need this hook
ast-grep --pattern 'export function $modalName() {
$$$
}'
Length of output: 588
Script:
#!/bin/bash
# Search for modal-related components and their implementations
rg -A 10 "export (function|const) .*(odal|Modal)" --type typescript
# Check if there are any other overlay-related hooks being used
rg "useOverlay|useModal" --type typescript
# Look for shouldBlockScroll prop usage
rg "shouldBlockScroll" --type typescript
Length of output: 260
Script:
#!/bin/bash
# Search for modal-related components and their implementations
rg -A 10 "export (function|const) .*(odal|Modal)"
# Check if there are any other overlay-related hooks being used
rg "useOverlay|useModal"
# Look for shouldBlockScroll prop usage
rg "shouldBlockScroll"
Length of output: 9758
packages/components/navbar/src/navbar-menu-toggle.tsx (1)
1-1: Ensure Compatibility with Updated useToggleButton Import
The import of useAriaToggleButton has been updated to import useToggleButton from @react-aria/button, aliased as useAriaToggleButton. Please verify that the new hook provides the same functionality and API as the previous useAriaToggleButton from @nextui-org/use-aria-toggle-button, to ensure there are no breaking changes.
✅ Verification successful
Import Change is Safe and Compatible
The change from @nextui-org/use-aria-toggle-button to @react-aria/button is safe as:
- The same types (
AriaToggleButtonProps) and hook functionality are being used - The hook usage pattern in the code remains identical
- The component implementation shows correct usage of the hook with the same parameter signature (
props, state, domRef) @react-aria/buttonis the original source of this hook, and@nextui-org/use-aria-toggle-buttonwas likely just a re-export
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of `useAriaToggleButton` to check for compatibility.
# Expectation: All usages should be compatible with the new hook.
rg 'useAriaToggleButton' -t tsx -A 5
Length of output: 64
Script:
#!/bin/bash
# Search for useAriaToggleButton usage without file type restriction
rg 'useAriaToggleButton' -A 5
# Also search for the original import path to understand the change
rg '@nextui-org/use-aria-toggle-button'
# Look for the specific file to examine its content
fd 'navbar-menu-toggle.tsx' --type f
Length of output: 3371
Script:
#!/bin/bash
# Let's examine the actual implementation to verify the hook usage
cat packages/components/navbar/src/navbar-menu-toggle.tsx
# Also check if there are any git changes to see what was replaced
git diff HEAD~1 packages/components/navbar/src/navbar-menu-toggle.tsx
Length of output: 3791
packages/hooks/use-aria-button/src/index.ts (2)
107-113: Deprecation Warning Implemented Correctly
The deprecation warning for onClick is properly implemented using the warn function from @nextui-org/shared-utils. This informs users to switch to onPress, aligning with the updated event handling approach.
114-124: Verify Safe Casting of Event Types in handlePress
In the handlePress function, the event e is cast as unknown before casting to React.MouseEvent<HTMLButtonElement>. Ensure that this type casting is safe and that the PressEvent is compatible with React.MouseEvent, to prevent any runtime errors or unexpected behavior.
✅ Verification successful
Type casting pattern is consistent across the codebase
The type casting from PressEvent to React.MouseEvent is a consistent pattern used across multiple components in the codebase, specifically in use-aria-link, use-aria-button, and use-menu-item. This pattern is deliberately used to maintain backwards compatibility for onClick handlers on mobile devices, as documented in the code comment referencing issue #4292. The consistent usage across different components suggests this is a well-established and tested approach within the codebase.
- Found in:
- packages/hooks/use-aria-link/src/index.ts
- packages/hooks/use-aria-button/src/index.ts
- packages/components/menu/src/use-menu-item.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the compatibility between `PressEvent` and `React.MouseEvent`.
# Expectation: Identify any differences that might cause issues.
rg 'interface PressEvent' -t ts -A 5
Length of output: 38
Script:
#!/bin/bash
# Let's check the imports and type definitions in the file
rg "PressEvent" -A 3 -B 3 packages/hooks/use-aria-button/src/index.ts
# Let's also check where this type might be imported from
rg "from.*PressEvent" packages/hooks/use-aria-button/src/index.ts
Length of output: 803
Script:
#!/bin/bash
# Let's check the definition of PressEvent in @react-types/shared
rg -g "*.d.ts" "PressEvent" node_modules/@react-types/shared/
# Let's also check if there are any similar type castings in the codebase that might serve as precedent
rg "as unknown as React\.MouseEvent" -t ts
Length of output: 568
packages/hooks/use-aria-link/package.json (1)
40-40: Addition of @nextui-org/shared-utils Dependency Looks Good
The addition of @nextui-org/shared-utils as a dependency is appropriate if the package utilizes shared utilities. Ensure that all necessary imports are updated accordingly.
packages/hooks/use-aria-button/package.json (1)
40-40: LGTM: Addition of shared-utils dependency
The addition of @nextui-org/shared-utils as a workspace dependency is appropriate for sharing common utilities across components.
packages/components/card/__tests__/card.test.tsx (1)
3-3: LGTM: UserEvent setup follows best practices
The setup of userEvent in a beforeEach block ensures consistent test behavior.
Also applies to: 8-12
packages/components/link/__tests__/link.test.tsx (1)
3-3: LGTM: UserEvent setup is consistent
The userEvent setup matches the implementation in card tests, maintaining consistency across the codebase.
Also applies to: 8-12
packages/components/button/__tests__/button.test.tsx (1)
39-48: LGTM! Good test coverage for backward compatibility
The test ensures that the deprecated onClick handler continues to work during the transition period, which is important for backward compatibility.
packages/hooks/use-aria-link/src/index.ts (3)
64-69: Good: Added deprecation warning for onClick
The warning message effectively guides users to migrate from onClick to onPress, with a reference to the relevant issue.
71-80: Verify mobile-specific event handling
The special handling for mobile devices ensures backward compatibility while transitioning to onPress. However, we should verify this behavior across different mobile browsers.
Consider adding this information to the migration guide to help users understand the mobile-specific behavior.
104-107: Good: Prevented double event firing on mobile
The check prevents onClick from firing twice on mobile devices where it's already handled in handlePress.
packages/components/link/src/use-link.ts (1)
40-45: Good: Clear deprecation notice in Props interface
The deprecation notice in the JSDoc comment clearly indicates that users should use onPress instead.
packages/components/link/stories/link.stories.tsx (1)
78-87: Good: Added interactive story example
The Pressable story effectively demonstrates the interactive behavior of the Link component.
packages/components/listbox/src/use-listbox-item.ts (2)
15-15: LGTM: Import statement is correctly updated
The warn utility is properly imported from shared utils to handle the deprecation warning.
49-49: LGTM: Deprecation warning implementation
The deprecation warning is well implemented with:
- Clear renaming of the prop to indicate deprecation
- Proper type checking before showing the warning
- Informative message with a link to the related issue
Also applies to: 71-76
packages/components/button/src/use-button.ts (1)
59-61: LGTM: Clear deprecation documentation
The deprecation is well documented in JSDoc with a clear instruction to use onPress instead.
packages/components/menu/src/use-menu-item.ts (4)
3-3: LGTM: Proper type imports
The PressEvent type is correctly imported for proper type safety in the handlePress implementation.
62-62: LGTM: Deprecation warning implementation
The deprecation warning is well implemented with proper type checking and clear messaging.
Also applies to: 85-90
92-98: LGTM: Robust event handling implementation
The handlePress callback is well implemented with:
- Proper event type conversion
- Performance optimization using useCallback
- Maintaining backward compatibility
114-114: LGTM: Proper integration with useAriaMenuItem
The handlePress callback is correctly passed to useAriaMenuItem's onPress prop.
packages/components/card/src/use-card.ts (1)
23-23: LGTM: Props interface correctly excludes onClick.
The use of Omit to exclude onClick from HTMLNextUIProps is the correct approach to prevent type conflicts with the deprecated onClick property.
packages/components/card/stories/card.stories.tsx (1)
439-445: LGTM: Story export is correctly implemented.
The Pressable story is properly exported with the correct template and default props.
| @@ -88,11 +88,18 @@ interface Props<T extends object = {}> extends Omit<ItemProps<"li", T>, "childre | |||
| classNames?: SlotsToClasses<MenuItemSlots>; | |||
| } | |||
|
|
|||
| export type MenuItemBaseProps<T extends object = {}> = Props<T> & | |||
| export type MenuItemBaseProps<T extends object = {}> = Omit<Props<T>, "onClick"> & | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace empty object type with a more specific type
The use of {} as a type is discouraged as it means "any non-nullable value". Consider using a more specific type or Record<string, unknown> if you need an empty object type.
-export type MenuItemBaseProps<T extends object = {}> = Omit<Props<T>, "onClick"> &
+export type MenuItemBaseProps<T extends object = Record<string, unknown>> = Omit<Props<T>, "onClick"> &📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export type MenuItemBaseProps<T extends object = {}> = Omit<Props<T>, "onClick"> & | |
| export type MenuItemBaseProps<T extends object = Record<string, unknown>> = Omit<Props<T>, "onClick"> & |
🧰 Tools
🪛 Biome (1.9.4)
[error] 91-91: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
| export type MenuItemBaseProps<T extends object = {}> = Omit<Props<T>, "onClick"> & | ||
| Omit<MenuItemVariantProps, "hasDescriptionTextChild" | "hasTitleTextChild"> & | ||
| AriaMenuItemProps & | ||
| FocusableProps & | ||
| PressEvents; | ||
| PressEvents & { | ||
| /** | ||
| * The native click event handler. | ||
| * use `onPress` instead. | ||
| * @deprecated | ||
| */ | ||
| onClick?: MouseEventHandler<HTMLLIElement | HTMLAnchorElement>; | ||
| }; |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent onClick deprecation across components
The codebase shows inconsistent handling of onClick vs onPress:
- Several components still use onClick without deprecation notices (Button, Card, Link, etc.)
- Only MenuItemBase shows signs of deprecation transition
- Many components have implemented onPress but maintain onClick without deprecation warnings
- No consistent pattern of deprecation notices found across components
Key locations requiring attention:
packages/components/button/src/use-button.ts: Has both onClick and onPress without deprecationpackages/components/card/src/use-card.ts: Has both onClick and onPress without deprecationpackages/components/link/src/use-link.ts: Only has onClick implementationpackages/components/listbox/src/base/listbox-item-base.tsx: Has onClick without deprecation notice
🔗 Analysis chain
Verify consistent onClick deprecation across components
Let's ensure this deprecation is consistently implemented across all interactive components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent onClick deprecation implementation across components
# Search for onClick props in component definitions
echo "Checking onClick usage in component definitions..."
rg -t typescript "onClick.*?:" packages/components/
# Search for deprecation notices
echo "Checking deprecation notices..."
rg -t typescript "@deprecated.*onClick" packages/components/
# Search for documentation references
echo "Checking documentation..."
rg -t markdown "onClick" docs/
Length of output: 508
Script:
#!/bin/bash
# Let's try a different approach to search for onClick usage and deprecation
# Search for onClick props in component definitions without file type restriction
echo "Checking onClick usage in component definitions..."
rg "onClick\s*[?]?:\s*(.*?)" packages/components/
# Search for deprecation notices
echo "Checking deprecation notices..."
rg "@deprecated.*onClick" packages/components/
# Search for onPress usage to compare
echo "Checking onPress usage..."
rg "onPress\s*[?]?:\s*(.*?)" packages/components/
# Look for any documentation files in the components directory
echo "Checking for documentation files..."
fd -e md . packages/components/
Length of output: 8129
🧰 Tools
🪛 Biome (1.9.4)
[error] 91-91: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
| if (type === "button" && isMobile) { | ||
| // Avoid firing onClick event twice since it's handled in handlePress | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reevaluate onClick Handler Condition for All Element Types
The condition if (type === "button" && isMobile) in the onClick handler aims to prevent the onClick event from firing twice on mobile devices. However, this may not cover elements with different type or elementType values. Consider adjusting the condition to ensure it applies to all relevant element types.
Apply this diff to make the condition more comprehensive:
- if (type === "button" && isMobile) {
+ if (isMobile) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (type === "button" && isMobile) { | |
| // Avoid firing onClick event twice since it's handled in handlePress | |
| return; | |
| } | |
| if (isMobile) { | |
| // Avoid firing onClick event twice since it's handled in handlePress | |
| return; | |
| } | |
| @@ -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"> & | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace empty object type with a more specific type
Using {} as a type is discouraged as it means "any non-nullable value". Consider defining a more specific interface or type.
-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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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"> & |
🧰 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)
| <Button | ||
| {...args} | ||
| aria-label={isOpen ? "Close" : "Open"} | ||
| aria-pressed={isOpen} | ||
| onClick={handlePress} | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update to use onPress instead of onClick
Since onClick is being deprecated in favor of onPress, the story should reflect this change to demonstrate the recommended usage pattern.
<Button
{...args}
aria-label={isOpen ? "Close" : "Open"}
aria-pressed={isOpen}
- onClick={handlePress}
+ onPress={handlePress}
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Button | |
| {...args} | |
| aria-label={isOpen ? "Close" : "Open"} | |
| aria-pressed={isOpen} | |
| onClick={handlePress} | |
| > | |
| <Button | |
| {...args} | |
| aria-label={isOpen ? "Close" : "Open"} | |
| aria-pressed={isOpen} | |
| onPress={handlePress} | |
| > |
| const handlePress = (e: any) => { | ||
| // eslint-disable-next-line no-console | ||
| console.log("Pressed"); | ||
| console.log("Pressed", e); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace any type with specific event type
Using any type reduces type safety. Since this is a button click event, we should use the specific event type.
-const handlePress = (e: any) => {
+const handlePress = (e: React.MouseEvent<HTMLButtonElement>) => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handlePress = (e: any) => { | |
| // eslint-disable-next-line no-console | |
| console.log("Pressed"); | |
| console.log("Pressed", e); | |
| const handlePress = (e: React.MouseEvent<HTMLButtonElement>) => { | |
| // eslint-disable-next-line no-console | |
| console.log("Pressed", e); |
| const handlePress = (e: any) => { | ||
| // eslint-disable-next-line no-console | ||
| console.log("Pressed", e); | ||
|
|
||
| setIsOpen(!isOpen); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and use onPress instead of onClick
The current implementation has several issues:
- Uses
anytype for the event parameter - Uses the deprecated onClick handler
- Contains a disabled eslint rule
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}>Committable suggestion skipped: line range outside the PR's diff.
| <DropdownItem key="new" onClick={() => alert("New file")}> | ||
| New file | ||
| </DropdownItem> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider removing the onClick handler in favor of onAction
The component already has an onAction handler at the Dropdown level that triggers an alert. Adding an onClick handler here will cause:
- Duplicate alerts since both handlers will fire
- Inconsistency with other items that only use
onAction
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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <DropdownItem key="new" onClick={() => alert("New file")}> | |
| New file | |
| </DropdownItem> | |
| <DropdownItem key="new"> | |
| New file | |
| </DropdownItem> |
| <ListboxItem key="new" onClick={() => alert("[onClick] New file")}> | ||
| New file | ||
| </ListboxItem> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider removing the onClick handler in favor of onAction
The component already has an onAction handler at the Listbox level that triggers an alert. Adding an onClick handler here will cause:
- Duplicate alerts since both handlers will fire
- Inconsistency with other items that only use
onAction
Remove the onClick handler since the action is already handled by onAction:
- <ListboxItem key="new" onClick={() => alert("[onClick] New file")}>
+ <ListboxItem key="new">
New file
</ListboxItem>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <ListboxItem key="new" onClick={() => alert("[onClick] New file")}> | |
| New file | |
| </ListboxItem> | |
| <ListboxItem key="new"> | |
| New file | |
| </ListboxItem> |
* ci: when build fail will exit the release process (#4319) * fix: on click event on interactive elements (#4322) * chore: deprecated style added to api table * ci(changesets): version packages (#4323) Co-authored-by: Junior Garcia <jrgarciadev@gmail.com> --------- Co-authored-by: winches <329487092@qq.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
After testing, passing the First, the event type for Second, I believe that Overall, the behavior of I'm not sure if the two situations I mentioned are due to a bug in Thank you for your work, but could you look into any other solutions? I don't want to keep the version at 2.6.7 because future versions may have other bug fixes that I am aware of. |
|
In version 2.6.7, when using In the image is 2.6.7, you can also see that the event is
|
Closes: #4292
📝 Description
This PR addresses issues related to the
onClickevent handling in various interactive components and introduces improvements by deprecatingonClickin favor ofonPress. Additionally, unnecessary packages have been removed, and React Aria internal utilities have been updated.⛳️ Current Behavior (Updates)
onClickevent is used across interactive components likeMenuItem,ListboxItem,Link, andButton.🚀 New Behavior
onClickevent forMenuItem,ListboxItem,Link,Button, and other interactive components to ensure better consistency.onClickin favor ofonPressfor interactive elements to align with modern accessibility standards.💣 Is this a breaking change (Yes/No): Yes
onClickevent will need to migrate toonPressas soon as possible.onClicktoonPressin your codebase.📝 Additional Information