Skip to content
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

enh(css-like) custom attributes should highlight as attr, including the -- #3237

Closed
thanoskrg opened this issue Jun 11, 2021 · 10 comments
Closed
Labels
bug help welcome Could use help from community language

Comments

@thanoskrg
Copy link
Contributor

Hello and thanks for this amazing library! I am using it for my blog to highlight code snippets, and recently I used CSS highlighting on a code example which included CSS variables.

More specifically I have this case:

/* default context vars */
body {
  --color: black;
  --background-color: lightgrey;
}

/* dark theme context vars */
body.dark {
  --color: lightgrey;
  --background-color: black;
}

/* selector with styles using vars */
main {
  color: var(--color);
  background-color: var(--background-color);
  transition: background-color 0.2s ease-in-out;
}

The output looks like this:

Screenshot 2021-06-11 at 10 05 44 AM

Normally, I would expect that the prefixing -- of the variables would have been handled as part of the attribute.

I am thinking of forking this repo to introduce a PR addressing this issue. What do you think?

@thanoskrg thanoskrg added bug help welcome Could use help from community language labels Jun 11, 2021
@joshgoebel
Copy link
Member

Any patch here would need to keep in mind #2269 and provide patches for ALL 4 grammars potentially or explain clearly why this does not apply to some (I'm not sure if it does or not). A patch that only fixes just CSS would not be accepted.

I'm not sure what to do here is so clear cut though. This reminds me a little of our refusal to highlight vendor prefixes, which I have argued is actually a feature, not a bug. There are a few different things to consider here:

  • Should we highlight -- as part of the name?
  • Should we use variable or attr or attribute? I think technically these would be variable.definition or some such.
  • Inside of var(--blah-blah) should we then use variable? (if we decide not to at the top-level)

While Github's choices are interesting I don't want to fall into the trap that it's somehow "canonical" or that we always must do the same thing just because they (or anyone else) does.

@allejo Thoughts?

@thanoskrg
Copy link
Contributor Author

Hey @joshgoebel, each one of these grammars, have their own syntax for defining variables.

My example is explicit for the --blah-blah syntax under the context of CSS highlighting. Regarding your bullet points:

Should we highlight -- as part of the name?

I would say yes.

Inside of var(--blah-blah) should we then use variable?

I don't think so.

Do you think it would make sense to open a PR as a proposal? I would really like to contribute to highlight.js library!

@joshgoebel
Copy link
Member

joshgoebel commented Jun 12, 2021

Hey @joshgoebel, each one of these grammars, have their own syntax for defining variables.

I'm not talking about custom variable systems that have been "bolted on". Most (all?) of these are (or allow) super-sets of CSS, ie they allow basic CSS without using any of the advanced features. In those cases simple CSS should highlight exactly the same across all 4 grammars when it's simple CSS. This has been all over the map in the past and a lot of work has been done to bring this back in line...

So when considering any changes to basic CSS all grammars that are supersets of CSS must ALSO be dealt with simultaneously.

@joshgoebel
Copy link
Member

Inside of var(--blah-blah) should we then use variable?

I don't think so.

Isn't that exactly what your PR does though? I'm confused.

@joshgoebel
Copy link
Member

joshgoebel commented Jun 12, 2021

Should we highlight -- as part of the name?

I would say yes.

I don't have strong opinions on this but technically it's not part of the variable name though... it's more "syntactic sugar" for declaring that an attribute is a variable vs a CSS attribute. But I suppose if we highlight @@module and @body as variables in Ruby (and similar in other grammars) that we already have precedent... although I'm not sure we're 100% consistent with this across the entire core library.

@allejo
Copy link
Member

allejo commented Jun 12, 2021

Declaring a custom property is done using a custom property name that begins with a double hyphen (--)

According to MDN docs, it sounds to me like -- along with the name is the custom property; it's not that -- is separate from the property name. Based off that, I think --my-variable should be highlighted as a single entity (e.g. "variable") in a ruleset and inside of a var() call.

Considering that these are "custom properties", they're no different from any other CSS property. So I think they should be highlighted with the same class name and have an additional class like variable or custom-property or whatever if themes want to opt-in to changing their colors.

@thanoskrg
Copy link
Contributor Author

Isn't that exactly what your PR does though? I'm confused.

Yes you right, and that's because I would get weird highlighting behavior otherwise. Here is an example of not handling the var(--blah-blah) how would look like:

weird-highlighting

It seems a bit off. What do you think guys? My PR addresses this on these lines:

{
begin: /\(/,
contains: [ CSS_VARIABLE ],
}

@joshgoebel
Copy link
Member

According to MDN docs, it sounds to me like -- along with the name is the custom property;

Thanks for bringing some much needed clarity. :-)

I think --my-variable should be highlighted as a single entity

Agree, but it's really an attr in that case (as you've described it)... an attribute but with no semantic meaning. It would not be a variable at the point of definition.

Considering that these are "custom properties", they're no different from any other CSS property.

Well, different in that they have no semantic meaning on their own, which per our our guidelines makes them attr vs attributes.

and have an additional class like variable or custom-property

I don't see how this is needed since this is already covered with the distinction between attr vs attribute.


It seems a bit off.

That is an entirely separate problem. To avoid that we can match it with a rule - but that doesn't mean we must give it a scope or highlight it any special way.

main {
  color: var(--color);

Now at this point we have a custom property being used as a variable... You could make the argument to apply variable to --color here, or not... We typically only use attr and attribute at the point of definition, so if we decided not to use variable here then --color should remain unhighlighted.

@joshgoebel
Copy link
Member

joshgoebel commented Jun 13, 2021

Since var is already highlighted (as a function like thingy) and pops I'm inclined to say it can remain unhighlighted (as GitHub is doing) rather than highlighting both var and the name.

So what I think needs to happen here:

  • Scope of PR is reduced to just fixing the highlighting issue with --custom-property, now highlighting the -- as well... and doing so as attr vs attribute.
  • This change needs to be applied to SCSS, Stylus, and Less grammars also as originally mentioned.
    • See tools/css and the css_consistency markup tests
    • Only a single small example need to added.

@joshgoebel joshgoebel changed the title CSS variables are not handled as expected enh(css-like) custom attributes should highlight as attr, including the -- Jun 13, 2021
@joshgoebel
Copy link
Member

Closing with PR closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug help welcome Could use help from community language
Projects
None yet
Development

No branches or pull requests

3 participants