refactor(docs): use onPress event instead #4353
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
WalkthroughThis pull request introduces modifications primarily focused on changing event handling from 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: 2
🧹 Outside diff range and nitpick comments (4)
apps/docs/app/examples/navbar/bordered/page.tsx (1)
Line range hint
44-48: Consider adding explicit onPress handler for the Sign Up buttonWhile the basic Link functionality works, consider adding an explicit onPress handler for better control and analytics tracking on the Sign Up action.
<Button as={Link} color="primary" href="#" variant="flat"> + onPress={(e) => { + // Add analytics tracking or additional handling here + console.log('Sign up clicked'); + }} Sign Up </Button>apps/docs/app/examples/modal/placement/page.tsx (1)
Line range hint
61-65: LGTM! Event handler migration completedThe change from
onClicktoonPresson the modal buttons is correct and aligns with NextUI's conventions. Consider updating the component's documentation to reflect this change.Consider adding a code comment to document that
onPressis the preferred event handler for NextUI components.apps/docs/app/examples/navbar/disable-menu-animation/page.tsx (2)
9-9: Systematic update needed for event handlingThis change is part of a pattern across all navbar examples. While removing the custom Link wrapper is a good simplification, the migration to
onPressevents is incomplete. Consider:
- Adding
onPresshandlers for interactive elements- Documenting the new event handling pattern in the examples
Would you like help creating a consistent pattern for handling press events across all navbar examples?
Line range hint
1-1: Global changes needed to align with PR objectivesThe current changes across all navbar examples show a consistent pattern of removing the custom Link wrapper, but don't fully implement the stated objective of migrating from
onClicktoonPress. Consider:
- Implementing proper
onPresshandlers where interaction is needed- Documenting the new event handling patterns in the examples
- Either maintaining preventDefault behavior or using proper routes instead of "#" hrefs
- Updating the PR description to accurately reflect the scope of changes
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
apps/docs/app/examples/modal/placement/page.tsx(1 hunks)apps/docs/app/examples/navbar/bordered/page.tsx(1 hunks)apps/docs/app/examples/navbar/controlled-menu/page.tsx(1 hunks)apps/docs/app/examples/navbar/custom-active-item/page.tsx(1 hunks)apps/docs/app/examples/navbar/disable-menu-animation/page.tsx(1 hunks)apps/docs/app/examples/navbar/disabled-blur/page.tsx(1 hunks)apps/docs/app/examples/navbar/hide-on-scroll/page.tsx(1 hunks)apps/docs/app/examples/navbar/static/page.tsx(1 hunks)apps/docs/app/examples/navbar/usage/page.tsx(1 hunks)apps/docs/app/examples/navbar/with-avatar/page.tsx(1 hunks)apps/docs/app/examples/navbar/with-dropdown-menu/page.tsx(1 hunks)apps/docs/app/examples/navbar/with-menu/page.tsx(1 hunks)apps/docs/app/examples/navbar/with-search-input/page.tsx(1 hunks)apps/docs/components/marketing/install-banner.tsx(2 hunks)apps/docs/components/marketing/sponsor-item.tsx(1 hunks)apps/docs/components/mdx-components.tsx(1 hunks)apps/docs/components/sandpack/copy-button.tsx(1 hunks)
🔇 Additional comments (15)
apps/docs/components/marketing/sponsor-item.tsx (1)
Line range hint 20-25: LGTM! Event handler change looks good.
The change from onClick to onPress is correctly implemented while preserving the PostHog tracking functionality.
Let's verify if there are any remaining onClick handlers in sponsor-related components:
✅ Verification successful
No remaining onClick handlers found in sponsor components
The absence of results from our search queries indicates that there are no remaining onClick handlers in sponsor-related components. The change from onClick to onPress appears to be complete and consistent across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining onClick handlers in sponsor-related files
rg -i "onClick" apps/docs/components/**/sponsor-*.{ts,tsx,js,jsx}
Length of output: 413
Script:
#!/bin/bash
# Let's try a more reliable search approach using fd to find sponsor-related files first
fd "sponsor.*\.(tsx?|jsx?)$" apps/docs/components/ -x rg -i "onClick" {}
Length of output: 74
apps/docs/components/sandpack/copy-button.tsx (1)
26-28: LGTM! Button event handler updated correctly.
The onPress event handler is properly implemented on the Button component while maintaining the copy functionality.
apps/docs/app/examples/navbar/usage/page.tsx (1)
3-3: Verify intended link behavior after removing custom Link component.
The import statement has been simplified by removing the custom Link component that prevented default behavior. Please confirm if this is the intended behavior for all navbar links.
Let's check for any remaining custom Link implementations:
apps/docs/app/examples/navbar/bordered/page.tsx (1)
3-3: Verify Button and Link component interaction
The import changes look good, but we should verify that the Button component properly handles events when used with as={Link}.
✅ Verification successful
Let me gather more information about the Button and Link components interaction.
Let me try a different approach to search for these components.
Let me search for the actual Link component implementation to verify its event handling.
Let me try one final search to find the polymorphic component handling.
Button with Link integration is properly handled
The Link component implementation shows proper event handling through useAriaLink hook which handles both onClick and onPress events. The Button component using as={Link} is a common pattern across multiple examples in the codebase, and the Link component is designed to work with polymorphic components through its proper event forwarding mechanism.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Button component implementation to verify event handling
ast-grep --pattern 'export const Button = $$$'
Length of output: 1692
Script:
#!/bin/bash
# Search for Button component implementation in the actual component files
rg -A 10 "export const Button = " --type typescript
# Search for any Button+Link usage patterns in the codebase
ast-grep --pattern 'Button as={Link}'
# Search for Link component implementation
rg -A 10 "export const Link = " --type typescript
Length of output: 215
Script:
#!/bin/bash
# Search for Button component implementation
rg -A 10 "export const Button"
# Search for Link component implementation
rg -A 10 "export const Link"
# Search for Button with Link usage examples
rg -A 3 "Button.*as=.*Link"
Length of output: 13244
Script:
#!/bin/bash
# Search for Link component in the components directory
fd Link.tsx -t f packages/components | xargs cat
# Search for any polymorphic component handling
rg -A 5 "as={" packages/components
Length of output: 159
Script:
#!/bin/bash
# Search for Link component implementation
fd -e tsx -e ts -t f -d 3 "link" packages/components/link/src | xargs cat
# Search for polymorphic component patterns
rg "type.*Props.*as.*=" packages/components
Length of output: 4614
apps/docs/app/examples/navbar/static/page.tsx (1)
3-3: LGTM! Changes are consistent with other navbar examples
The import changes align with the PR objectives and maintain consistency across components.
apps/docs/app/examples/navbar/hide-on-scroll/page.tsx (1)
3-3: LGTM! Verify scroll behavior with event handling
The import changes are consistent. Since this component uses shouldHideOnScroll, we should verify that the scroll behavior doesn't interfere with link/button event handling.
apps/docs/app/examples/navbar/disabled-blur/page.tsx (1)
3-3: Verify Link component event handling behavior
The import change from custom NextUiLink to the standard Link component is aligned with the migration to onPress events. However, we should verify the event handling behavior in nested components.
✅ Verification successful
Let me gather more information about the Link component implementation and its event handling.
Let me check one more thing about the AriaLinkProps to understand the complete event handling interface.
Import change is safe and correctly implemented
The verification shows that the Link component from @nextui-org/react is properly used in the navbar example without any onClick handlers. The component is used consistently for both regular links and button-styled links, following the library's patterns. The implementation aligns with the migration to onPress events, and there are no compatibility issues to address.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining onClick handlers in Link components
ast-grep --pattern 'Link $props={$$$} onClick={$_}'
# Search for documentation or type definitions of Link component
rg -A 5 "export (type|interface) LinkProps"
Length of output: 476
Script:
#!/bin/bash
# Check the Link component implementation for event handling
ast-grep --pattern 'const Link = $_ => {
$$$
}'
# Search for UseLinkProps definition to understand available props
rg -A 10 "export (type|interface) UseLinkProps"
# Search for useLink hook implementation
ast-grep --pattern 'export const useLink = $_ => {
$$$
}'
Length of output: 2913
Script:
#!/bin/bash
# Search for AriaLinkProps definition and its event handlers
rg -A 5 "type AriaLinkProps"
# Check if there are any onClick handlers in the navbar example
ast-grep --pattern 'onPress={$_}' apps/docs/app/examples/navbar/disabled-blur/page.tsx
# Check the actual implementation of the navbar example
cat apps/docs/app/examples/navbar/disabled-blur/page.tsx
Length of output: 1696
apps/docs/app/examples/navbar/custom-active-item/page.tsx (1)
3-3: LGTM! Import changes are consistent
The changes align with the standardization of event handling while preserving the custom active item styling functionality.
apps/docs/app/examples/navbar/with-avatar/page.tsx (1)
8-8: LGTM: Import statement updated correctly
The direct import of Link from @nextui-org/react aligns with the migration from onClick to onPress events, as the updated component internally handles the new event system.
apps/docs/app/examples/navbar/with-search-input/page.tsx (1)
8-8: LGTM: Import statement updated correctly
The direct import of Link from @nextui-org/react is consistent with the migration to the new event system.
apps/docs/app/examples/navbar/with-dropdown-menu/page.tsx (2)
8-8: LGTM: Import statement updated correctly
The direct import of Link from @nextui-org/react aligns with the migration to onPress events.
Line range hint 119-121: Verify Button and Link interaction with onPress event
The Button as={Link} pattern needs verification to ensure proper event handling with the new onPress system.
✅ Verification successful
Let me generate additional scripts to check for the implementation details of the Button and Link components to understand how they handle events.
Based on the analysis of the codebase, I can now provide a final response:
Button as={Link} pattern is properly handled with the new onPress system
The codebase shows that both Button and Link components are using React Aria's press event system consistently:
- The Button component uses
useAriaButtonhook which properly handles bothonClickandonPressevents - The Link component uses
useAriaLinkhook with similar event handling - Both hooks include mobile-specific handling and proper event chaining
- The pattern is consistently used across multiple examples in the codebase
- No conflicting event handling patterns were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for similar Button as={Link} patterns in the codebase
# and verify consistent event handling
# Search for Button components using Link
rg -l "Button.*as=\{Link\}" | while read -r file; do
echo "=== $file ==="
rg -A 2 "Button.*as=\{Link\}" "$file"
done
# Search for any explicit onClick handlers on Button or Link
rg -l "onClick.*Button|onClick.*Link" | while read -r file; do
echo "=== $file ==="
rg -A 2 "onClick.*Button|onClick.*Link" "$file"
done
Length of output: 10076
Script:
#!/bin/bash
# Check Button and Link component implementations
ast-grep --pattern 'export const Button = $$$'
# Check for any onPress related code
rg -A 3 "onPress.*Button|onPress.*Link"
# Check for any event handling in Button or Link components
rg -A 3 "handlePress|pressHandler"
Length of output: 31657
apps/docs/components/marketing/install-banner.tsx (1)
Line range hint 75-82: LGTM! Event handlers updated correctly.
The onClick handlers have been properly replaced with onPress while maintaining the analytics tracking functionality.
Let's verify if there are any remaining onClick handlers in the component:
Also applies to: 94-101
✅ Verification successful
LGTM! Event handlers are correctly implemented
The verification confirms that:
- All click handlers are using
onPress - No remaining
onClickhandlers exist in the component - Both "Get Started" and "Github" buttons are properly using
onPresswith their respective analytics tracking
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining onClick handlers in the InstallBanner component
ast-grep --pattern 'onClick={$_}' apps/docs/components/marketing/install-banner.tsx
Length of output: 85
Script:
#!/bin/bash
# Let's first check if the file exists and view its content
cat apps/docs/components/marketing/install-banner.tsx
# Also search for any click-related handlers with ripgrep for broader coverage
rg "(?i)(onclick|onpress)" apps/docs/components/marketing/install-banner.tsx
Length of output: 4564
apps/docs/components/mdx-components.tsx (2)
201-201: LGTM! Event handler updated correctly.
The onClick handler has been properly replaced with onPress while maintaining the analytics tracking functionality.
Let's verify keyboard accessibility and remaining onClick handlers:
✅ Verification successful
Keyboard accessibility is properly handled by NextUI's Link component
After examining the code, the change from onClick to onPress is safe because:
- The Link component uses NextUI's Link component which inherently provides keyboard accessibility
- The component spreads external props including
targetandrelattributes for proper external link handling - The analytics tracking is preserved through the
handleClickfunction
The NextUI Link component internally handles keyboard interactions, making explicit onKeyDown handlers unnecessary. The component maintains both accessibility and analytics requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining onClick handlers in the MDX components
ast-grep --pattern 'onClick={$_}' apps/docs/components/mdx-components.tsx
# Check if the Link component has proper keyboard handling
ast-grep --pattern 'onKeyDown={$_}' apps/docs/components/mdx-components.tsx
Length of output: 153
Script:
#!/bin/bash
# Let's examine the Link component implementation more thoroughly
cat apps/docs/components/mdx-components.tsx
Length of output: 11332
201-201: Consider verifying keyboard accessibility.
Since we're switching from onClick to onPress, ensure that keyboard navigation (Enter key) still works correctly with the Link component. The NextUI Link component should handle this internally, but it's worth verifying.
Let's check the NextUI Link component implementation:
✅ Verification successful
Keyboard accessibility is properly handled by NextUI Link component
The implementation is using NextUI's Link component which inherently handles keyboard interactions. The code shows:
- The Link is rendered using
Components.Linkfrom "@nextui-org/react" - It maintains proper accessibility by:
- Using semantic anchor elements
- Preserving proper href attributes
- Including proper external link attributes when needed (target, rel)
- The onPress handler is only used for analytics tracking and doesn't interfere with the default keyboard behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for keyboard event handling in NextUI Link component
rg -A 5 'onKeyDown|onPress' node_modules/@nextui-org/react/
Length of output: 133
Script:
#!/bin/bash
# Let's check the actual implementation in the codebase
rg -A 5 "onPress.*handleClick" apps/docs/components/mdx-components.tsx
# Let's also check if there are any keyboard event handlers
rg -A 5 "onKeyDown|onKeyPress" apps/docs/components/mdx-components.tsx
# Let's see the Link component usage context
ast-grep --pattern 'Link {
$$$
onPress={$_}
$$$
}'
Length of output: 286
Script:
#!/bin/bash
# Let's check the Link component import and its source
rg "import.*Link.*from" apps/docs/components/mdx-components.tsx
# Let's also see the full Link component implementation
ast-grep --pattern 'const Components.Link = ({ $$$, children }) => {
$$$
}'
Length of output: 145
Script:
#!/bin/bash
# Let's see the entire content of the file to understand the Link component implementation
cat apps/docs/components/mdx-components.tsx
Length of output: 11332
| @@ -5,17 +5,14 @@ import { | |||
| NavbarBrand, | |||
| NavbarContent, | |||
| NavbarItem, | |||
| Link as NextUiLink, | |||
| LinkProps, | |||
| Link, | |||
There was a problem hiding this comment.
💡 Codebase verification
Links in example code should use proper routes instead of "#" placeholders
Based on the verification:
- All links in the navbar examples use "#" as href values
- There are no onClick/onPress handlers or preventDefault calls
- These are example components meant to showcase the navbar functionality
Since this is documentation example code, using "#" hrefs without proper routing or preventDefault behavior could lead to unexpected page jumps, affecting the demo experience. The links should either:
- Use meaningful demo routes (e.g., "/features", "/customers")
- Or include a note in the documentation that these are placeholder links
🔗 Analysis chain
Verify the intended behavior of link navigation
The removal of the custom Link wrapper that prevented default behavior means these links will now attempt to navigate. However, since all href values are set to "#", this will cause the page to jump to the top. Consider either:
- Maintaining the preventDefault behavior for demo purposes
- Using proper routes instead of "#" in the href attributes
Let me check if there are any event handlers (onClick/onPress) on these links to understand the complete behavior.
Let me check the previous implementation to understand what was removed.
Let me try a different approach to find any custom link components or wrappers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any actual routes defined for these links
rg "href=|to=" apps/docs/app/examples/navbar
Length of output: 5886
Script:
#!/bin/bash
# Check for any onClick or onPress handlers on links in the navbar examples
rg -A 1 "<Link.*?>" apps/docs/app/examples/navbar/with-menu/page.tsx
Length of output: 343
Script:
#!/bin/bash
# Check the git history for the file to see the previous implementation
git log -p apps/docs/app/examples/navbar/with-menu/page.tsx | grep -A 10 "^-.*Link"
Length of output: 0
Script:
#!/bin/bash
# Look for any custom Link components or wrappers in the codebase
fd -e tsx -e ts "Link" apps/docs/app/examples/navbar --exec grep -l "Link"
# Also check for any preventDefault usage
rg "preventDefault" apps/docs/app/examples/navbar
Length of output: 122
| @@ -9,13 +9,10 @@ import { | |||
| NavbarMenuItem, | |||
| NavbarMenuToggle, | |||
| Button, | |||
| Link as NextUiLink, | |||
| LinkProps, | |||
| Link, | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider implementing onPress handlers for interactive elements
While removing the custom Link wrapper simplifies the code, interactive elements should use onPress instead of relying on default link behavior, especially in a controlled menu context.
Closes #
📝 Description
replace deprecated
onClickdue to #4322⛳️ Current behavior (updates)
🚀 New behavior
💣 Is this a breaking change (Yes/No):
📝 Additional Information
Summary by CodeRabbit
New Features
onPressinstead ofonClick, enhancing interaction tracking.Bug Fixes
Linkcomponent, improving navigation consistency.Documentation
Chores