Open benhatsor opened 3 years ago
This probably needs to wait until the css_consistency
branch is merged or it needs to be implemented on top of that branch. I don't imagine it would be a hard fix though.
I also plan to merge that branch into master soon and then no more long lived feature branches. :)
Will do. I'll close the issue when it merges.
Oh I'm not saying that will fix it... I'm not sure... only that any fix should be made on that branch, not master.
I am looking into pushing a fix to css_consistency. First, do we want to highlight 'animation' as a variable?
First, do we want to highlight 'animation' as a variable?
Well, I think that could make sense. But then we also have actual CSS variables, so I'm not sure if those should be colored the same as keyframe identifers... so I think I'm learning towards no?
Not to mention many of the CSS like grammars have their own idea of variables also, so lets say no.
A PR here would likely just fix the number highlighting glitch I think.
FYI css_consistency
is rebased and merged into master now.
Unflagged as good first issue, I have a feeling this one might be sneaky since it's going to be contextual in some grammars I think.
@joshgoebel I have created a PR to the best of my knowledge. It solves this number issue.
On review I'm not 100% sure we should highlight this as a number. Here is VS Code:
The advantage being that is we don't style from/to that we have consistency. Or maybe you could argue to/from are numbers in this context, but that seems confusing - and adds more complexity to every grammar.
CC @allejo
Currently our grammars are all over the map.
Less calls them all tags, which I'm pretty sure is not [semantically] correct:
They were just trying to add a "splash of color". I do see how it very roughly makes sense from a purely visual perspective... but if we decide to stick with that we'd likely name these and then use a cssAlias to pull it back to tag
.
So perhaps tag
for from and to and number for %
? I think if we color number we probably should color from/to and if we don't color from/to we probably shouldn't color number.
I guess I'd be ok with using variable
as well (for now) though we may leave room to change that also in the future, perhaps keyframeName
and then alias back to variable
. This will likely require a separate rule in every grammar just to handle @keyframes
, but that might not be terrible.
Reference: https://github.com/highlightjs/highlight.js/issues/2269
From what I see both of the following options seem okay.
from/to
and %
. This means going the vscode route.from/to
as a tag and %
as a number.We can choose whichever option keeps things simple.
We'll see if @allejo has any thoughts. I been working on the CSS consistency more and this is pretty simple once you flatten the grammar... you can just use from/to keywords at the top-level for the frame positives, easy peasy. So complexity shouldn't stop us.
I don't think we should be treating these keyframe "waypoints" as numbers or tags; nor should from
and to
be highlighted as numbers even though they are just aliases for 0%
and 100%
. They're not numerical values like you'd see elsewhere in CSS, they're more so "keys" in a dictionary.
MDN and WebStorm highlight them as "selectors."
MDN
(Source: https://developer.mozilla.org/en-US/docs/Web/CSS/animation-fill-mode#CSS)
WebStorm
So I'd lean towards not highlighting them at all or highlighting them as we would selectors. Which is picked, both percentages and from
/to
keywords should be highlighted the same.
What kind of selector? :-)
selector-tag
selector-id
selector-class
selector-attr
selector-pseudo
LOL. What I showed above was selector-tag
, which I feel like if we were just going to pick one it seems OK.
I'm kind of leaning towards NOT highlighting them now though. The nice thing about treating them DIFFERENTLY is you can just let the global rules do their thing... if you want to treat them the same now we need a MODE just to handle @keyframes
(and match to, from, NUMBER inside it - but hack number to be highlighted with a different class) and we're back to parsing more understand context.
But we might have to do something anyways to avoid it being treated as a selector (which is why this bug was originally filed)... I'll take a quick pass to see about doing "nothing" and if that's impossible then we'll add the rule we need to have the context to highlight them all the same, and just deal with it.
@allejo I looked into this more. I'm not sure that doing either of what you'd prefer (and what might be actually be best) is even possible. We have to handle cases like this in Stylus:
@keyframes pop
0%
opacity:0
transform:scale(.5)
50%
opacity:1
100%
opacity:1
transform:scale(1)
We're not a parser... so significant whitespace means nothing to us... so there is simply no way to treat those 50%
any differently than we'd treat any other. We simply can't KNOW that we're inside the scope of @keyframes (to give them special treatment). Of course we could still choose to highlight to
and from
(or not)... but I feel like the frame % ship might have sailed. Unless I'm missing something?
The whole idea is to consolidate our CSS/Stylus/SCSS/Less support to be more consistent (since they can all support raw CSS)... even with advanced features they should still be very consistent in terms of class names, visual feel, etc... so if Stylus has no way to do this, then I'm not sure we should attempt it in CSS and SCSS just because we have {}
context there... esp. when it makes the grammars more complex (and non-consistent).
It may be important to note there are severals problems here.
:
to handle values)50.1%
results in the css class rule detecting .1
as a class selector.And bingo... just hit me. I think a negative look-ahead on the class selector to negate %
might do the trick. :) I'll give it a shot. Ugh, and the actual bug here was just regex that's too wide... I don't think .1
is even a valid class.
.1 is highlighted as a hljs-selector-class.
@barhatsor Can you test master? I can't confirm this bug you've reported (at least not with CSS):
Less and SCSS both seem problematic (their regex is too broad), but CSS doesn't seem to.
.1 is highlighted as a hljs-selector-class.
@barhatsor Can you test master? I can't confirm this bug you've reported (at least not with CSS):
I just tested it. The problem @barhatsor mentioned is there in the css_consistency
branch but not in the master
.
Oh? css_consistency
was rebased and merged.... master includes it now.
@allejo Did you ever really explain why highlighting the percent as a # and the other different is bad to you? Is it just a consistency/semantic thing? I meant technically 50% is a number, a number used to specify the keyframe placement... My suggestion to highlight to/from as numbers was never really serious. :)
It looks like the two easy choices here are no highlighting or DIFFERENT highlighting to to/from vs numbers... I'm ok with choosing none but then we'll have to close the PR @il3ven opened.
Of course Stylus allows things like:
$green_dark = darken(
$green,
10)
$size = 10px
So now we'd need special rules in Stylus for:
func(
(recursive)Just to allow numbers to be highlighted in those circumstances (but not the more general case of appearing globally, inside @keyframes)
CSS animations highlighting incorrectly When writing CSS animations, such as:
It highlights incorrectly.
animation
isn't highlighted as a variable10.1%
isn't highlighted as anhljs-number
as it should be.1
is highlighted as ahljs-selector-class
.Language: CSS
I used
highlight
...
Sample Code to Reproduce
Or any other CSS animation, really. Example JSFiddle: https://jsfiddle.net/barhatsor/oz14ewyx/2/
Expected behavior Turns out Github also higlights it incorrectly so here's a picture of CodeMirror getting it right:
Would be happy if this got fixed. Thanks!