Skip to content

fix: restore default cursor and add commitment in docs#1261

Merged
alexdln merged 7 commits intonpmx-dev:mainfrom
kalu5:fix/#1074
Feb 13, 2026
Merged

fix: restore default cursor and add commitment in docs#1261
alexdln merged 7 commits intonpmx-dev:mainfrom
kalu5:fix/#1074

Conversation

@kalu5
Copy link
Contributor

@kalu5 kalu5 commented Feb 9, 2026

Restore the return cursor and delete the cursor of the basic button.

@vercel
Copy link

vercel bot commented Feb 9, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
npmx.dev Ready Ready Preview, Comment Feb 13, 2026 7:03pm
2 Skipped Deployments
Project Deployment Actions Updated (UTC)
docs.npmx.dev Ignored Ignored Preview Feb 13, 2026 7:03pm
npmx-lunaria Ignored Ignored Feb 13, 2026 7:03pm

Request Review

@codecov
Copy link

codecov bot commented Feb 9, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 9, 2026

📝 Walkthrough

Walkthrough

This pull request removes the cursor-pointer utility class from the Button component's root element in app/components/Button/Base.vue and adds a new "Cursor and navigation" section to CONTRIBUTING.md. The documentation section prescribes cursor behaviour: use cursor-pointer for links and the default cursor for other interactive elements, defines criteria for meaningful links versus non-meaningful ones, and emphasises favouring real link elements for stable URLs. The new documentation content is present twice in the file.

Possibly related PRs

Suggested reviewers

  • whitep4nth3r
🚥 Pre-merge checks | ✅ 1 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Merge Conflict Detection ⚠️ Warning ⚠️ Unable to check for merge conflicts: Invalid branch name format
✅ Passed checks (1 passed)
Check name Status Explanation
Description check ✅ Passed The pull request description accurately describes the core changes: removing cursor-pointer from the basic button and updating documentation regarding cursor behavior.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
⚔️ Resolve merge conflicts (beta)
  • Auto-commit resolved conflicts to branch fix/#1074
  • Post resolved changes as copyable diffs in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Comment on lines 53 to 56
<button
type="button"
class="inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:outline-accent/70 shrink-0"
class="cursor-pointer inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:outline-accent/70 shrink-0"
@click="router.back()"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Remove per-button focus-visible utility (global rule already covers it).

The back button still carries focus-visible:outline-accent/70, but focus-visible styling for buttons is already applied globally.

Suggested change
-            class="cursor-pointer inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:outline-accent/70 shrink-0"
+            class="cursor-pointer inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded shrink-0"

Based on learnings: “In the npmx.dev project, ensure that focus-visible styling for button and select elements is implemented globally in app/assets/main.css … Do not apply per-element inline utility classes like focus-visible:outline-accent/70.”

📝 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.

Suggested change
<button
type="button"
class="inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:outline-accent/70 shrink-0"
class="cursor-pointer inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:outline-accent/70 shrink-0"
@click="router.back()"
<button
type="button"
class="cursor-pointer inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded shrink-0"
`@click`="router.back()"

Comment on lines 118 to 121
<button
type="button"
class="inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:outline-accent/70 shrink-0"
class="cursor-pointer inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:outline-accent/70 shrink-0"
@click="router.back()"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Remove per-button focus-visible utility (global rule already covers it).

The back button still carries focus-visible:outline-accent/70, but focus-visible styling for buttons is already applied globally.

Suggested change
-            class="cursor-pointer inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:outline-accent/70 shrink-0"
+            class="cursor-pointer inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded shrink-0"

Based on learnings: “In the npmx.dev project, ensure that focus-visible styling for button and select elements is implemented globally in app/assets/main.css … Do not apply per-element inline utility classes like focus-visible:outline-accent/70.”

📝 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.

Suggested change
<button
type="button"
class="inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:outline-accent/70 shrink-0"
class="cursor-pointer inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:outline-accent/70 shrink-0"
@click="router.back()"
<button
type="button"
class="cursor-pointer inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded shrink-0"
`@click`="router.back()"

Comment on lines 30 to 33
<button
type="button"
class="inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:outline-accent/70 shrink-0"
class="cursor-pointer inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:outline-accent/70 shrink-0"
@click="router.back()"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Remove per-button focus-visible utility (global rule already covers it).

The back button still carries focus-visible:outline-accent/70, but focus-visible styling for buttons is already applied globally.

Suggested change
-            class="cursor-pointer inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:outline-accent/70 shrink-0"
+            class="cursor-pointer inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded shrink-0"

Based on learnings: “In the npmx.dev project, ensure that focus-visible styling for button and select elements is implemented globally in app/assets/main.css … Do not apply per-element inline utility classes like focus-visible:outline-accent/70.”

📝 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.

Suggested change
<button
type="button"
class="inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:outline-accent/70 shrink-0"
class="cursor-pointer inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:outline-accent/70 shrink-0"
@click="router.back()"
<button
type="button"
class="cursor-pointer inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded shrink-0"
`@click`="router.back()"

@kalu5
Copy link
Contributor Author

kalu5 commented Feb 9, 2026

I suggest adding the description of this problem to the contribution document to remind everyone to consider it when adding the cursor.
Also pay attention to this issue during the review to avoid style overwriting.
What do you think? @alexdln

@kalu5
Copy link
Contributor Author

kalu5 commented Feb 9, 2026

I found that the default style of Vue Data UI also uses "pointer". I'll submit an issue first.

@essenmitsosse
Copy link
Contributor

I think the consensus was that all clickable things should get cursor: pointer. You can still add cursor-pointer to the plain buttons that don't have it yet, but this will also be fixed once I replace them with the ButtonBase component.

@kalu5
Copy link
Contributor Author

kalu5 commented Feb 9, 2026

I think the consensus was that all clickable things should get cursor: pointer. You can still add cursor-pointer to the plain buttons that don't have it yet, but this will also be fixed once I replace them with the ButtonBase component.

Something like this: It is agreed that all buttons should use a unified component, such as ButtonBase. This component uses the default style. You can also use cursor: pointer to make it a Link Button.

@essenmitsosse
Copy link
Contributor

No I meant, also all buttons are supposed to just keep cursor-pointer. I think no one wanted an extra treatment for non-link buttons.

@kalu5
Copy link
Contributor Author

kalu5 commented Feb 9, 2026

No I meant, also all buttons are supposed to just keep cursor-pointer. I think no one wanted an extra treatment for non-link buttons.

Here is a discussion. #1074

@whitep4nth3r
Copy link
Contributor

To make this cursor-pointer discussion a final final, can this PR include some guidelines about it summing up the discussions and decisions please?

Rules about:

  • what's defined in global CSS
  • when should cursor pointer be used
  • how to treat the ButtonBase component when it is a button and a link

Thank you!

@danielroe danielroe requested a review from alexdln February 12, 2026 15:57
@danielroe
Copy link
Member

yes! apologies for the slow review but I think we need to set some consensus here or I fear we'll have alternating PRs reverting/implementing cursor 😆

@essenmitsosse
Copy link
Contributor

  • how to treat the ButtonBase component when it is a button and a link

ButtonBase is always a button. LinkBase can look like a button.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

@alexdln alexdln changed the title fix: fix cursor miss fix: restore default cursor and add commitment in docs Feb 13, 2026
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

@alexdln alexdln enabled auto-merge February 13, 2026 19:14
@alexdln
Copy link
Member

alexdln commented Feb 13, 2026

There are still some questions, but since we haven't made a final decision and the trigger-elements currently have different cursors, I'm merging it and leaving a new section in contributing guide that we use default cursor for buttons

We'll work through it further and, if necessary, we will update the contributing guide

Thanks for fixing ❤️

@alexdln alexdln added this pull request to the merge queue Feb 13, 2026
Merged via the queue into npmx-dev:main with commit 391d28e Feb 13, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants