-
Notifications
You must be signed in to change notification settings - Fork 3.7k
(chore) updates nnfx
theme for v11
#3187
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
Conversation
@@ -11,7 +11,8 @@ | |||
color: #fff; | |||
} | |||
|
|||
.xml .hljs-meta { | |||
.language-xml .hljs-meta, | |||
.language-xml .hljs-meta-string { |
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.
Ah, I should go back and look for this in other themes... I thought I remembered taking care of it but obviously I missed some. Thanks!
src/styles/nnfx-dark.css
Outdated
@@ -24,7 +25,8 @@ | |||
} | |||
|
|||
.hljs-name, | |||
.hljs-keyword { | |||
.hljs-keyword, | |||
.hljs-attribute { |
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.
Given some snippet:
var bob : Person = {
name: "John",
age: 32
}
var
and name
and age
are all highlighted the same as keywords. Is this your intent?
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.
Thanks for the feedback. Yes, it's intentional. The idea of attribute here is to pick up css property names. I think var
, name
and age
all the same is fine, though. As it turns out, at least for (inline) JavaScript, name
and age
are picked up as attr rather than attribute, so end up looking different.
That said, please do keep the feedback coming. I run the checkTheme.js
tool and look at the samples in the demo to make sure nothing dreadful has happened, but that isn't exhaustive by any means, so I appreciate your pointing out potential issues like this.
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 idea of attribute here is to pick up css property names.
If that was the ONLY idea then you could scope it as much: .language-css, .language-less
, etc... Just letting you know that attribute
is much broader than "CSS attribute".
As it turns out, at least for (inline) JavaScript, name and age are picked up as attr rather than attribute
Yes, because the keys have no semantic meaning. There is a reason I didn't say my snippet was javascript. :-) Some grammars do have attributes with semantic meaning though side-by-side with keywords... see csp
, qml
, etc.
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.
Updated the snippet to make them attributes
more clearly. :)
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 principal use of the theme is browser view-source (to mirror what Firefox does), so it focuses mainly on html/xml and embedded css and JavaScript.
As for attribute, I understand it is more than just css. I'm debating whether to keep it or not for different reasons, though. The parser doesn't pick up some css properties well (e.g., in the case of -webkit-border-radius, it identifies 'border-radius' as attribute and the rest as nothing, so it looks wonky.)
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.
in the case of -webkit-border-radius, it identifies 'border-radius' as attribute and the rest as nothing
This is intentional. Vendor specific prefixes are "unusual" and should look different.
The principal use of the theme is browser view-source (to mirror what Firefox does), so it focuses mainly on html/xml and embedded css and JavaScript.
Yes I recall now we discussed this long ago. It's tough though because there is no way to communicate that intent to users selecting the theme from a list of themes. Users will expect themes to work well with many different languages, not just "web stuff". So that will always be a consideration for themes inside core I think.
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.
in the case of -webkit-border-radius, it identifies 'border-radius' as attribute and the rest as nothing
This is intentional. Vendor specific prefixes are "unusual" and should look different.
All things considered, I'm backing out attribute styling, so it will be unadorned as it was previously.
As an aside, the css demo has a custom property --heading-1
. The 1 is parsed as number and the rest as nothing. As with vendor properties, this seems somehow wrong IMO.
src/styles/nnfx-light.css
Outdated
@@ -24,7 +25,8 @@ | |||
} | |||
|
|||
.hljs-name, | |||
.hljs-keyword { | |||
.hljs-keyword, | |||
.hljs-attribute { |
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.
Oh you added attribute here also. :-) Maybe I wait until you not WIP. :-)
Removing WIP, I'm happy. Let's go. |
I lie, I pushed one more change... that should be it! ;-) [edit: on->one] |
nnfx
theme for v11
@RocketMan Thanks so much! |
This includes a few minor changes in support of v11.
Note: This is a work-in-progress. Will remove WIP tag when final.