-
-
Notifications
You must be signed in to change notification settings - Fork 410
Don't add cursor: pointer
to var
s when variable highlighting is inactive
#4870
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?
Conversation
Thanks for the PR @twiss. Just to clear my understanding: With var highlighting on, I think we agree we should continue showing |
I think #3765 would be a great addition, yeah, though I think it's in principle orthogonal from this change indeed in the sense that that change should be made in src/core/highlight-vars.js (by removing the |
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.
Pull Request Overview
This PR removes the cursor: pointer
CSS property from var
elements that don't have interactive functionality in exported documents, improving the user experience by avoiding misleading cursor behavior.
- Restricts the positioning CSS rule to only apply to
var
elements with adata-type
attribute - Removes the
cursor: pointer
property entirely from the datatype CSS module - Relies on the existing var.css.js module to handle cursor styling for interactive variable highlighting
@@ -3,9 +3,8 @@ const css = String.raw; | |||
// Prettier ignore only to keep code indented from level 0. | |||
// prettier-ignore | |||
export default css` | |||
var { | |||
var[data-type] { |
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.
The CSS selector change from var
to var[data-type]
may create inconsistency if the position: relative
property is needed for all var
elements, not just those with data-type attributes. Consider whether this positioning change could affect the layout of var
elements without data-type attributes.
var[data-type] { | |
var { |
Copilot uses AI. Check for mistakes.
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.
This isn't needed, the position: relative
is only to help position the position: absolute
::before
and ::after
pseudoelements below.
✅ Deploy Preview for respec-pr ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
In exported documents, variable highlighting is inactive, and clicking on
var
s doesn't do anything. So, I think we shouldn't setcursor: pointer
on them.This seems to possibly have been a mistake and only intended for variables with a data type hover bubble, but even there, clicking on them doesn't do anything so setting
cursor: pointer
on them still doesn't particularly make sense, IMHO.For non-exported documents when highlighting is active, src/styles/var.css.js will still set
cursor: pointer
, so this shouldn't break that case.