Set cursor-pointer for button #6775
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
I stumbled upon this in the tree view where the toolbar icons don't behave like most other buttons all across the UI. I did not check all possible places where Button is used, but can confirm that it works on the toolbar there and also for example on the scratchpad. |
|
This is pretty opinionated, so I'm not sure what the 'right' default is. But it makes sense to me, would love to get others' thoughts too. A better change is to do this in tailwind so it applies to all buttons: https://tailwindcss.com/docs/upgrade-guide#buttons-use-the-default-cursor. Probably in |
|
I'd prefer the tailwind default (it is similar how the OS works as well). Im am going to close, but feel free to keep the discussion going @sebkur if you have strong opinions. Regardless, we appreciate the contribution! |
|
OK, I'd appreciate to understand this. If I get it right, adding the pointer cursor in more places is kind of the wrong direction. Instead for consistency the buttons that do already have a pointer cursor should be changed to show the regular arrow pointer? Is this kind of consistency desired? I'm attaching a short video that shows the pointer cursor on some UI elements that I think could then be changed to arrow: marimo-pointer.mp4 |
|
@sebkur thats a good callout. I can let some folks on the team have input |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| const buttonVariants = cva( | ||
| cn( | ||
| "disabled:opacity-50 disabled:pointer-events-none", | ||
| "disabled:opacity-50 disabled:pointer-events-none cursor-pointer", |
There was a problem hiding this comment.
Bug: Disabled Buttons Show Incorrect Cursor
The cursor-pointer class is applied to all buttons, including disabled ones. This causes disabled buttons to display a pointer cursor, which is misleading UX since disabled:pointer-events-none prevents interaction.
There was a problem hiding this comment.
The cursor comment is a good point. Disabled buttons shouldn't show a cursor pointer.
|
@sebkur , We agreed this would be a good change to make. We should use the tailwind solution instead. |
sebkur
force-pushed
the
button-with-pointer
branch
from
86913e4 to
198a7e8
Compare
sebkur
force-pushed
the
button-with-pointer
branch
from
95866d5 to
256756b
Compare
Unlike many other clickable items in the UI, some buttons do not change the cursor from default (arrow) to pointer on hover. In accordance with the Tailwind Upgrade Guide from v3 to v4 (see https://tailwindcss.com/docs/upgrade-guide#buttons-use-the-default-cursor), apply a rule in the base layer for all non-disabled buttons to use the pointer cursor. Some buttons across the code base also had CSS class 'cursor-pointer' applied explicitly, which is now no longer needed and have been removed.
sebkur
force-pushed
the
button-with-pointer
branch
from
256756b to
0e872c8
Compare
for more information, see https://pre-commit.ci
|
Alright, thanks @Light2Dark! I applied that solution from the Tailwind upgrade guide now. I searched for buttons that got the |
| <button | ||
| className={cn( | ||
| "flex items-center p-2 text-sm mx-px shadow-inset font-mono cursor-pointer rounded", | ||
| "flex items-center p-2 text-sm mx-px shadow-inset font-mono rounded", |
There was a problem hiding this comment.
Here, the sidebar is a good example, it no longer needs cursor-pointer here now.
5 participants
📝 Summary
Unlike other clickable items in the UI, the button does not have the cursor-pointer CSS class. As a result hovering buttons in various places does not change the cursor from arrow to pointer.
🔍 Description of Changes
I added the
cursor-pointerCSS class to the Button component📋 Checklist