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

feat(css): handle css variables syntax #3239

Merged
merged 4 commits into from Jun 22, 2021

Conversation

thanoskrg
Copy link
Contributor

@thanoskrg thanoskrg commented Jun 12, 2021

This PR addresses the #3237 so that CSS variables syntax is properly highlighted.

Changes

  • Update src/languages/css.js definition
  • Update test scenarios for CSS markup

Checklist

  • Added markup tests
  • Updated the changelog at CHANGES.md

@joshgoebel
Copy link
Member

A visual snap is alwyas nice with PRs like this.

src/languages/css.js Outdated Show resolved Hide resolved
@thanoskrg
Copy link
Contributor Author

A visual snap is alwyas nice with PRs like this.

@joshgoebel here it is how it looks like on base16/nord theme.

demo-css-variables

@thanoskrg thanoskrg marked this pull request as ready for review June 13, 2021 13:52
@thanoskrg
Copy link
Contributor Author

That's how it looks now after the latest changes:

demo-css-variables-2

@joshgoebel
Copy link
Member

Theme maintainers: Here is a case where we're increasing the semantic density of our highlighting, but at the same time perhaps making existing theme behavior worse. I have a suspicion many/some themes only think about attribute when it comes to CSS... and haven't given any thought to attr at all (in the context of CSS). Given how some themes almost contain "micro-themes" for different types of markup (Markdown vs diffs vs Code). I'm not sure of the perfect solution.

Just sticking with attribute here for theme behavior has the best short-term behavior but I'm not sure is a great long-term plan. Perhaps the real problem is our historic distinction between attr and attribute... already having a clear semantic scope to reach for precludes something more complex like attribute.custom... thought perhaps some would argue that's better than breaking themes in the short-term? Though it also (in the short or mid-term) makes the distinction meaningless until some future themes comes along to express the styles differently.

I could also see many themes wanting to just merge these two styles (in the case of CSS), yes I hesitate to do that at the grammar level since I always try to be as accurate as possible when it comes to scopes - and then let themes decide HOW to highlight those scope.

So perhaps the semantic difference here between attributes and custom attributes (specifically w.r.t. CSS) just doesn't matter? Thoughts?

CC @highlightjs/theme-maintainers @highlightjs/core

@thanoskrg
Copy link
Contributor Author

@joshgoebel any updates on this? Is this currently blocked until we get an answer on your last question?

@joshgoebel
Copy link
Member

I was hoping for some thoughts, yes. Eventually if there is nothing this'll probably merge as-is... I don't usually get in a rush until I have a new release in sight. I need to go figure out when 11.1 will be.

@thanoskrg
Copy link
Contributor Author

I was hoping for some thoughts, yes. Eventually if there is nothing this'll probably merge as-is... I don't usually get in a rush until I have a new release in sight. I need to go figure out when 11.1 will be.

Thanks for your swift reply.

@thanoskrg thanoskrg force-pushed the handle-css-variables branch 3 times, most recently from 83e5a7d to 90c7e9f Compare June 19, 2021 11:01
@joshgoebel joshgoebel merged commit 0f6814c into highlightjs:main Jun 22, 2021
@thanoskrg thanoskrg deleted the handle-css-variables branch June 22, 2021 07:03
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.

None yet

2 participants