feat: use cursor-pointer on buttons & CmdPalette#2752
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds the ChangesCursor Pointer Styling
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
Hello! Thank you for opening your first PR to npmx, @aryanpingle! 🚀 Here’s what will happen next:
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/components/Header/AccountMenu.client.vue`:
- Line 228: The account menu has inconsistent pointer affordance: ensure all
menuitem buttons in the AccountMenu.vue component use the same clickable classes
by adding the missing cursor-pointer to the connected-account menuitem buttons
(the ones that currently have class="w-full text-start gap-x-3 border-none" at
the other two menu entries) so they match the menuitem that uses class="w-full
text-start gap-x-3 border-none cursor-pointer"; update the class attribute on
those menuitem buttons to include cursor-pointer so all items behave
consistently.
In `@app/components/Package/ListToolbar.vue`:
- Line 181: In ListToolbar.vue remove the inline focus-visible utility classes
from the button element that currently includes "focus-visible:ring-2
focus-visible:ring-fg focus-visible:ring-offset-2 focus-visible:ring-offset-bg";
update the element's class attribute to drop those four tokens so the button
relies on the global focus-visible rules defined in main.css, leaving the other
utilities (p-2.5, rounded-md, border, bg-bg-subtle, text-fg-muted,
hover:text-fg, hover:border-border-hover, transition-colors, duration-200,
cursor-pointer) intact.
In `@app/components/Package/WeeklyDownloadStats.vue`:
- Line 406: Remove the inline focus-visible utility from the button class list
in WeeklyDownloadStats.vue: the class token "focus-visible:outline-accent/70"
should be deleted from the ButtonBase/button element's class attribute so it
relies on the global focus-visible rule defined in main.css; search for the
ButtonBase render (button element) and strip that specific token from its class
string and verify no other inline focus-visible utilities remain in the
component.
In `@app/components/ViewModeToggle.vue`:
- Line 19: The cursor-pointer class is only on the inactive state string—move it
out of the conditional state classes and into the base class for both buttons in
ViewModeToggle.vue so both active and inactive buttons are consistently
interactive; locate the class bindings on the two toggle buttons (the template
lines using the ternary class strings around 'text-fg-muted' / active classes)
and remove 'cursor-pointer' from the conditional branches and add it to the
shared/base class string for each button.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cb101137-b249-4d64-9b9b-81885659ccff
📒 Files selected for processing (13)
app/components/AppHeader.vueapp/components/Header/AccountMenu.client.vueapp/components/Header/MobileMenu.client.vueapp/components/InstantSearch.vueapp/components/Modal.client.vueapp/components/Package/DownloadButton.vueapp/components/Package/ListToolbar.vueapp/components/Package/Versions.vueapp/components/Package/WeeklyDownloadStats.vueapp/components/SearchProviderToggle.client.vueapp/components/ViewModeToggle.vueapp/pages/index.vueapp/pages/package/[[org]]/[name].vue
| v-if="!isNpmConnected" | ||
| role="menuitem" | ||
| class="w-full text-start gap-x-3 border-none" | ||
| class="w-full text-start gap-x-3 border-none cursor-pointer" |
There was a problem hiding this comment.
Apply the same pointer affordance to all account menu menuitem buttons.
These two updates are good, but the connected account menuitem buttons in the same menu (Line 145 and Line 188) are still clickable without cursor-pointer, so the menu remains inconsistent.
Suggested minimal follow-up
- class="w-full text-start gap-x-3 border-none"
+ class="w-full text-start gap-x-3 border-none cursor-pointer"- class="w-full text-start gap-x-3 border-none"
+ class="w-full text-start gap-x-3 border-none cursor-pointer"Also applies to: 254-254
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/components/Header/AccountMenu.client.vue` at line 228, The account menu
has inconsistent pointer affordance: ensure all menuitem buttons in the
AccountMenu.vue component use the same clickable classes by adding the missing
cursor-pointer to the connected-account menuitem buttons (the ones that
currently have class="w-full text-start gap-x-3 border-none" at the other two
menu entries) so they match the menuitem that uses class="w-full text-start
gap-x-3 border-none cursor-pointer"; update the class attribute on those
menuitem buttons to include cursor-pointer so all items behave consistently.
| v-if="!searchContext || currentSort.key !== 'relevance'" | ||
| type="button" | ||
| class="p-2.5 rounded-md border border-border bg-bg-subtle text-fg-muted hover:text-fg hover:border-border-hover transition-colors duration-200 focus-visible:ring-2 focus-visible:ring-fg focus-visible:ring-offset-2 focus-visible:ring-offset-bg" | ||
| class="p-2.5 rounded-md border border-border bg-bg-subtle text-fg-muted hover:text-fg hover:border-border-hover transition-colors duration-200 focus-visible:ring-2 focus-visible:ring-fg focus-visible:ring-offset-2 focus-visible:ring-offset-bg cursor-pointer" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Remove inline focus-visible utilities on button element.
The inline focus-visible classes (focus-visible:ring-2, focus-visible:ring-fg, focus-visible:ring-offset-2, focus-visible:ring-offset-bg) should not be applied directly to button elements. The global CSS rule in main.css provides consistent focus-visible styling for all buttons.
♻️ Proposed fix to rely on global focus-visible styling
- class="p-2.5 rounded-md border border-border bg-bg-subtle text-fg-muted hover:text-fg hover:border-border-hover transition-colors duration-200 focus-visible:ring-2 focus-visible:ring-fg focus-visible:ring-offset-2 focus-visible:ring-offset-bg cursor-pointer"
+ class="p-2.5 rounded-md border border-border bg-bg-subtle text-fg-muted hover:text-fg hover:border-border-hover transition-colors duration-200 cursor-pointer"Based on learnings: In the npmx.dev project, focus-visible styling for button and select elements is implemented globally in app/assets/main.css. Do not apply per-element inline utility classes on these elements; rely on the global rule for consistency and maintainability.
📝 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.
| class="p-2.5 rounded-md border border-border bg-bg-subtle text-fg-muted hover:text-fg hover:border-border-hover transition-colors duration-200 focus-visible:ring-2 focus-visible:ring-fg focus-visible:ring-offset-2 focus-visible:ring-offset-bg cursor-pointer" | |
| class="p-2.5 rounded-md border border-border bg-bg-subtle text-fg-muted hover:text-fg hover:border-border-hover transition-colors duration-200 cursor-pointer" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/components/Package/ListToolbar.vue` at line 181, In ListToolbar.vue
remove the inline focus-visible utility classes from the button element that
currently includes "focus-visible:ring-2 focus-visible:ring-fg
focus-visible:ring-offset-2 focus-visible:ring-offset-bg"; update the element's
class attribute to drop those four tokens so the button relies on the global
focus-visible rules defined in main.css, leaving the other utilities (p-2.5,
rounded-md, border, bg-bg-subtle, text-fg-muted, hover:text-fg,
hover:border-border-hover, transition-colors, duration-200, cursor-pointer)
intact.
| type="button" | ||
| @click="openChartModal" | ||
| class="text-fg-subtle hover:text-fg transition-colors duration-200 inline-flex items-center justify-center min-w-6 min-h-6 -m-1 p-1 focus-visible:outline-accent/70 rounded" | ||
| class="text-fg-subtle hover:text-fg transition-colors duration-200 inline-flex items-center justify-center min-w-6 min-h-6 -m-1 p-1 focus-visible:outline-accent/70 rounded cursor-pointer" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Remove inline focus-visible utility on button element.
The inline focus-visible:outline-accent/70 class should not be applied directly to ButtonBase (which renders as a button element). The global CSS rule in main.css provides consistent focus-visible styling for all buttons.
♻️ Proposed fix to rely on global focus-visible styling
- class="text-fg-subtle hover:text-fg transition-colors duration-200 inline-flex items-center justify-center min-w-6 min-h-6 -m-1 p-1 focus-visible:outline-accent/70 rounded cursor-pointer"
+ class="text-fg-subtle hover:text-fg transition-colors duration-200 inline-flex items-center justify-center min-w-6 min-h-6 -m-1 p-1 rounded cursor-pointer"Based on learnings: In the npmx.dev project, focus-visible styling for button and select elements is implemented globally in app/assets/main.css. Individual buttons or selects in Vue components should not rely on inline focus-visible utility classes; rely on the global rule.
📝 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.
| class="text-fg-subtle hover:text-fg transition-colors duration-200 inline-flex items-center justify-center min-w-6 min-h-6 -m-1 p-1 focus-visible:outline-accent/70 rounded cursor-pointer" | |
| class="text-fg-subtle hover:text-fg transition-colors duration-200 inline-flex items-center justify-center min-w-6 min-h-6 -m-1 p-1 rounded cursor-pointer" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/components/Package/WeeklyDownloadStats.vue` at line 406, Remove the
inline focus-visible utility from the button class list in
WeeklyDownloadStats.vue: the class token "focus-visible:outline-accent/70"
should be deleted from the ButtonBase/button element's class attribute so it
relies on the global focus-visible rule defined in main.css; search for the
ButtonBase render (button element) and strip that specific token from its class
string and verify no other inline focus-visible utilities remain in the
component.
| viewMode === 'cards' | ||
| ? 'bg-bg-subtle text-fg border-fg-subtle' | ||
| : 'text-fg-muted hover:text-fg border-transparent' | ||
| : 'text-fg-muted hover:text-fg border-transparent cursor-pointer' |
There was a problem hiding this comment.
Make cursor behaviour consistent across both toggle states.
cursor-pointer is currently applied only when a mode is inactive. Since both buttons remain interactive, move cursor-pointer to the base class so active and inactive states behave consistently.
Suggested adjustment
- class="flex items-center px-2.5 py-2 text-sm font-medium rounded-sm border transition-colors duration-200 focus-visible:ring-2 focus-visible:ring-fg focus-visible:ring-offset-1"
+ class="flex items-center px-2.5 py-2 text-sm font-medium rounded-sm border transition-colors duration-200 focus-visible:ring-2 focus-visible:ring-fg focus-visible:ring-offset-1 cursor-pointer"- : 'text-fg-muted hover:text-fg border-transparent cursor-pointer'
+ : 'text-fg-muted hover:text-fg border-transparent'(Apply the same pattern to both buttons.)
Also applies to: 34-34
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/components/ViewModeToggle.vue` at line 19, The cursor-pointer class is
only on the inactive state string—move it out of the conditional state classes
and into the base class for both buttons in ViewModeToggle.vue so both active
and inactive buttons are consistently interactive; locate the class bindings on
the two toggle buttons (the template lines using the ternary class strings
around 'text-fg-muted' / active classes) and remove 'cursor-pointer' from the
conditional branches and add it to the shared/base class string for each button.
gameroman
added
the
needs discussion
graphieros
left a comment
graphieros
left a comment
There was a problem hiding this comment.
@aryanpingle 🙏 thank you for your PR, it's been a while indeed since the decision was taken to have the pointer on buttons.
I think the PR should solve all the cases to be merged, to avoid a partial situation which would remain confusing for users. Since this is my opinion, and perhaps other maintainers have a different one, I'm just commenting and not requesting changes.
|
@graphieros I get what you mean about the partial solution problem, in that it may be confusing for users to see some buttons have For a comprehensive fix across all buttons, there are two scenarios:
So from a tech standpoint, it's either easily revertible or a first step. Happy to hear any thoughts! |
|
@aryanpingle I feel well aligned with your approach. |
3 participants
🔗 Linked issue
Partially resolves #1760
🧭 Context
This commit removed
cursor: pointerfromButtonBase. The reason being, the W3C spec says:The commit was on Valentine's Day, but it is far from being something I love.
CONTRIBUTING.mdsays:And the linked issue states in its conclusion:
There was a similar PR (#2303) that attempted to do this only for the Download button.
And another PR (#2202) which was closed for potential spam.
To make things worse, at the moment some buttons and dropdowns arbitrarily have
cursor: pointerenabled, while more deserving buttons don't:cursor: pointerAND a fancy mousepress animationIf we're being inconsistent, let's at least err on the side of a better user experience.
I get that the plan was to adhere to the specification / default behaviour, but consider this: if I cared about being "official" more than the UX, I would be using npmjs.com instead of this awesome site.
📚 Description
This PR adds
cursor: pointerto some of the pure buttons that one is likely to encounter in the first few pages. Normal buttons + the command palette. My philosophy being, "A pointer cursor indicates that something will happen on clicking the element".I specifically didn't add it to the dropdowns in case that's another point of contention. I just wanna get this shipped soon so I can enjoy it as an end-user.
W3C this, W3C that. WHO CARES, THE POINTER IS WATCHING. I feel like I'm taking crazy pills, but I'm not because I ran out of crazy pills, I need more.