-
-
Notifications
You must be signed in to change notification settings - Fork 232
fix: fix cursor miss #1261
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix: fix cursor miss #1261
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughThis change refactors cursor-pointer utility class usage across button and interactive elements. The cursor-pointer class is removed from the Button/Base.vue component root styling, and instead added selectively to specific button instances in about.vue, compare.vue, privacy.vue, and settings.vue pages. Additionally, cursor-pointer is removed from a search provider select element in settings.vue. The net effect shifts cursor indication responsibility from the base button component level to individual page implementations. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this 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
| <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()" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| <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()" |
| <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()" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| <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()" |
| <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()" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| <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()" |
| <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 p-1.5 -mx-1.5" | ||
| 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 p-1.5 -mx-1.5" | ||
| @click="router.back()" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 enforced globally. Please drop the inline utility to keep consistency.
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 p-1.5 -mx-1.5"
+ 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 p-1.5 -mx-1.5"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.
| <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 p-1.5 -mx-1.5" | |
| 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 p-1.5 -mx-1.5" | |
| @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 p-1.5 -mx-1.5" | |
| `@click`="router.back()" |
|
I suggest adding the description of this problem to the contribution document to remind everyone to consider it when adding the cursor. |
|
I found that the default style of Vue Data UI also uses "pointer". I'll submit an issue first. |
|
I think the consensus was that all clickable things should get cursor: pointer. You can still add |
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. |
|
No I meant, also all buttons are supposed to just keep |
Here is a discussion. #1074 |
Restore the return cursor and delete the cursor of the basic button.