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

Classes hljs-attribute vs hljs-attr mismatch #1472

Closed
feimosi opened this issue Mar 11, 2017 · 3 comments
Closed

Classes hljs-attribute vs hljs-attr mismatch #1472

feimosi opened this issue Mar 11, 2017 · 3 comments

Comments

@feimosi
Copy link

feimosi commented Mar 11, 2017

highlight.js used to append hljs-attribute class in the past, but now it's hljs-attr. Unfortunately styles are not updated to match that. I've spotted it after migrating from 8.1 to 9.10.
See
https://github.com/isagalaev/highlight.js/blob/master/src/languages/xml.js#L14
and
https://github.com/isagalaev/highlight.js/blob/master/src/styles/tomorrow-night.css#L37

@isagalaev
Copy link
Member

isagalaev commented Mar 11, 2017

There was a huge overhaul of the style rules for 9.0, and the change you're describing is an intentional decision. We now have both attr and attribute with slightly different semantics, and attribute names within XML tags get the "less important" attr which is not highlighted in many styles by choice.

Having said that, if you want to advocate Tomorrow in particular should have a different color for "attr", we should pick a color that doesn't break other usages of this class name in [crmsh fix gcode haml htmlbars ini javascript json nix puppet xml yaml].

@feimosi
Copy link
Author

feimosi commented Mar 11, 2017

Well, I didn't know that, but still I reported the issue because styling after the update is worse for my HTML snippets.

For comparison, here's how it used to look like:
image
Here's how the new Tomorrow-Night looks like:
image

If we just change the CSS rule to use hljs-attr instead of hljs-attribute, we can achieve:
image
Note: on this example I also 'fixed' < >

@joshgoebel
Copy link
Member

Closing this:

  • It was an intentional change.
  • It has received very little discussion (so most must be ok with the change or not care)
  • Adjusting the CSS is always a fairly easy possibility.

Trying to clear out old issues.

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

No branches or pull requests

3 participants