Fix the Hover state issue (#531)#532
Conversation
WalkthroughAdds the CSS class "cursor-pointer" to numerous interactive UI elements across the frontend (buttons, menu items, switches, triggers). No changes to logic, state, handlers, APIs, or exports. All edits are presentation-only within className strings. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/components/ui/button.tsx (1)
8-8: Guard the disabled state cursorAdding
cursor-pointerglobally forces the pointer even when the button is disabled, which now communicates the item is still actionable. Please pair this withdisabled:cursor-not-allowed(or similar) so the disabled state continues to signal non-interactivity.- "inline-flex items-center justify-center gap-2 whitespace-nowrap rounded-md text-sm font-medium transition-all cursor-pointer disabled:pointer-events-none disabled:opacity-50 [&_svg]:pointer-events-none [&_svg:not([class*='size-'])]:size-4 shrink-0 [&_svg]:shrink-0 outline-none focus-visible:border-ring focus-visible:ring-ring/50 focus-visible:ring-[3px] aria-invalid:ring-destructive/20 dark:aria-invalid:ring-destructive/40 aria-invalid:border-destructive", + "inline-flex items-center justify-center gap-2 whitespace-nowrap rounded-md text-sm font-medium transition-all cursor-pointer disabled:pointer-events-none disabled:cursor-not-allowed disabled:opacity-50 [&_svg]:pointer-events-none [&_svg:not([class*='size-'])]:size-4 shrink-0 [&_svg]:shrink-0 outline-none focus-visible:border-ring focus-visible:ring-ring/50 focus-visible:ring-[3px] aria-invalid:ring-destructive/20 dark:aria-invalid:ring-destructive/40 aria-invalid:border-destructive",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/Navigation/Navbar/Navbar.tsx(2 hunks)frontend/src/components/ui/button.tsx(1 hunks)
🔇 Additional comments (2)
frontend/src/components/Navigation/Navbar/Navbar.tsx (2)
45-45: Clear-search button UX improvement looks goodThe explicit
cursor-pointerreads well here; it reinforces the close affordance without side effects.
62-62: Search trigger hover affordance confirmedThe pointer addition matches the component intent and keeps the interaction consistent with the shared button variant.
ShivaGupta-14
changed the title
github-actions
bot
added
UI
bug
There was a problem hiding this comment.
This is a ShadCN file; we should not change a standard component.
|
Got it, makes sense, since it’s a ShadCN file, we shouldn’t touch it directly. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx (1)
124-133: Cursor-pointer may be redundant on Button components.The ShadCN Button component typically applies
cursor: pointerby default for interactive button elements. Explicitly addingcursor-pointervia className is harmless but may be unnecessary and could create inconsistency across the codebase where some buttons have it explicitly and others rely on the default styling.frontend/src/components/Updater/UpdateDialog.tsx (2)
146-146: Cursor-pointer may be redundant on Button components.The ShadCN Button component typically applies
cursor: pointerby default. Explicitly addingcursor-pointermay be unnecessary here.
154-154: Cursor-pointer may be redundant on Button components.Same as the previous button—explicit
cursor-pointermay be unnecessary since the Button component should handle this by default.frontend/src/pages/SettingsPage/components/UserPreferencesCard.tsx (1)
46-49: Cursor-pointer may be redundant on Button component.The Button component typically has
cursor: pointerby default, making this explicit addition potentially unnecessary.frontend/src/components/Dialog/FaceSearchDialog.tsx (3)
77-77: Cursor-pointer may be redundant on Button component.The Button component should apply
cursor: pointerby default. Explicit addition here may be unnecessary.
96-104: Cursor-pointer may be redundant on Button component.Same as above—the Button component should handle cursor styling by default.
109-117: Cursor-pointer may be redundant on Button component.Same as above—explicit
cursor-pointermay be unnecessary for Button components.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
frontend/src/components/Dialog/FaceSearchDialog.tsx(3 hunks)frontend/src/components/Dialog/InfoDialog.tsx(1 hunks)frontend/src/components/FolderPicker/AITaggingFolderPicker.tsx(1 hunks)frontend/src/components/FolderPicker/DeleteImageDialog.tsx(1 hunks)frontend/src/components/FolderPicker/FolderPicker.tsx(1 hunks)frontend/src/components/Media/ImageCard.tsx(2 hunks)frontend/src/components/Navigation/Navbar/Navbar.tsx(1 hunks)frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx(1 hunks)frontend/src/components/OnboardingSteps/FolderSetupStep.tsx(1 hunks)frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx(1 hunks)frontend/src/components/ThemeToggle.tsx(1 hunks)frontend/src/components/Updater/UpdateDialog.tsx(1 hunks)frontend/src/components/ui/sidebar.tsx(1 hunks)frontend/src/pages/PersonImages/PersonImages.tsx(3 hunks)frontend/src/pages/SettingsPage/components/ApplicationControlsCard.tsx(2 hunks)frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx(2 hunks)frontend/src/pages/SettingsPage/components/UserPreferencesCard.tsx(4 hunks)
✅ Files skipped from review due to trivial changes (3)
- frontend/src/components/FolderPicker/DeleteImageDialog.tsx
- frontend/src/pages/PersonImages/PersonImages.tsx
- frontend/src/components/Media/ImageCard.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/components/Navigation/Navbar/Navbar.tsx
- frontend/src/components/ThemeToggle.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx (1)
frontend/src/components/ui/button.tsx (1)
Button(59-59)
frontend/src/components/ui/sidebar.tsx (1)
frontend/src/lib/utils.ts (1)
cn(5-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Tauri Build Check (windows-latest)
- GitHub Check: Tauri Build Check (macos-latest, --target aarch64-apple-darwin)
- GitHub Check: Tauri Build Check (ubuntu-22.04)
- GitHub Check: Backend Tests
🔇 Additional comments (3)
frontend/src/components/ui/sidebar.tsx (1)
267-267: Customizing local ShadCN UI components is intended; addingcursor-pointertoSidebarTriggeris fine.Likely an incorrect or invalid review comment.
frontend/src/pages/SettingsPage/components/UserPreferencesCard.tsx (2)
57-88: Good addition for DropdownMenuItem elements.Adding
cursor-pointerto DropdownMenuItem elements is appropriate, as these components may not have pointer cursor styling by default.
110-110: Good addition for Switch component.Adding
cursor-pointerto the Switch component improves the UX by clearly indicating interactivity, as Switch components may not have pointer cursor by default.
| <CardFooter className="flex justify-end p-3"> | ||
| <Button | ||
| className="px-4 py-1 text-sm" | ||
| className="cursor-pointer px-4 py-1 text-sm" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consider a centralized approach for cursor-pointer styling.
You asked in the PR description whether to use a wrapper/custom button or className at each usage. This PR takes the className-at-each-usage approach, which requires manually adding cursor-pointer in ~20+ places across the codebase. This creates maintenance burden and is error-prone.
Consider these alternatives:
- Preferred: Extend the base Button component to include
cursor-pointerby default in its variant styles - Alternative: Create a custom Button wrapper that adds the cursor styling and re-export it
- Investigate root cause: Most modern UI libraries style buttons with pointer cursor by default—this repetitive need might indicate a Tailwind or component configuration issue
Would you like me to help generate a centralized solution that eliminates the need for these repetitive changes?
🤖 Prompt for AI Agents
In frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx around line
133 the button uses className="cursor-pointer px-4 py-1 text-sm" and the review
calls out that adding cursor-pointer across ~20 places is repetitive; fix by
centralizing the pointer cursor: update the base Button component (or create a
small Button wrapper that re-exports the base) to include "cursor-pointer" in
its default variant/className so all consumers inherit it, then remove the
manual cursor-pointer classes from usages (including this file). Also verify
Tailwind/button reset isn't disabling pointer by default and run a quick
project-wide replace to remove redundant cursor-pointer strings, then smoke-test
UI and update any stories/tests if present.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Fetch all comments on this PR to see rahulharpal1603's guidance
gh pr view 532 --repo AOSSIE-Org/PictoPy --json comments --jq '.comments[] | "Comment by: \(.author.login)\nCreated: \(.createdAt)\nBody:\n\(.body)\n---"'
# Also fetch the original issue #531 to understand context
gh issue view 531 --repo AOSSIE-Org/PictoPy --json body,comments --jq '{body: .body, comments: [.comments[] | {author: .author.login, body: .body}]}'
Length of output: 19068
@ShivaGupta-14, understood! If @rahulharpal1603 advised using the className approach, then that makes sense for your codebase architecture.
The className-at-each-usage approach is perfectly valid, especially if there are reasons not to modify the base ShadCN Button component. Your team knows the codebase best, and consistency with existing patterns is important.
The current PR implementation looks good then—you've systematically added cursor-pointer where needed. Thanks for clarifying the team's decision!
🧬 Code Graph Analysis Results
: frontend/src/features/onboardingSlice.ts, lines 25-42
setAvatar(state, action: PayloadAction<string>) {
state.avatar = action.payload;
}
setName(state, action: PayloadAction<string>) {
state.name = action.payload;
}
markCompleted(state, action: PayloadAction<number>) {
const stepIndex = action.payload;
if (stepIndex >= 0 && stepIndex < state.stepStatus.length) {
state.stepStatus[stepIndex] = true;
} else {
console.warn(
`Invalid step index: ${stepIndex}. Valid range: 0-${state.stepStatus.length - 1}`,
);
}
state.currentStepIndex = state.stepStatus.findIndex((status) => !status);
state.currentStepName = STEP_NAMES[state.currentStepIndex] || '';
}
: frontend/src/constants/avatars.ts, lines 1-10
export const avatars = [
'/avatars/avatar1.png',
'/avatars/avatar2.png',
'/avatars/avatar3.png',
'/avatars/avatar4.png',
'/avatars/avatar5.png',
'/avatars/avatar6.png',
'/avatars/avatar7.png',
'/avatars/avatar8.png',
];
: frontend/src/components/ui/button.tsx, lines 59-59
Button
: frontend/src/components/ui/label.tsx, lines 22-22
Label
: frontend/src/components/ui/card.tsx, lines 90-90
CardDescription
: frontend/src/components/ui/card.tsx, lines 87-87
CardFooter
|
@rahulharpal1603 could you please review it? |
2 participants
Solves Issue #531
Applied cursor-pointer style to:
Note: Other components like NetflixStylePlayer and 2 more could also benefit from this improvement in future commits.(Need Clearance from your side.)
Summary by CodeRabbit