Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Improve the support for @keyframes #158

Merged
merged 6 commits into from
Aug 8, 2016
Merged

Improve the support for @keyframes #158

merged 6 commits into from
Aug 8, 2016

Conversation

hediyi
Copy link
Contributor

@hediyi hediyi commented Aug 1, 2016

fixes #157

@hediyi
Copy link
Contributor Author

hediyi commented Aug 2, 2016

@keyframes is actually a CSS feature, maybe I should change all the *.scss to *.css?

'name': 'meta.at-rule.keyframes.scss'
'patterns': [
{
'match': '(?<=keyframes[ \\t])(?:[_A-Za-z][-\\w]|-[_A-Za-z])[-\\w]*'
Copy link
Contributor

@winstliu winstliu Aug 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not liking this lookbehind because it doesn't take into account multiple spaces.

Also I think the other half can be rewritten as (?:-?[_A-Za-z])[-\\w]* right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it can be, if you don't want to impose the "must contain at least 2 characters" restriction, coz it also matches something like _.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(?:-?[_A-Za-z])[-\\w]+ then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😆 That imposes a new restriction that if the identifier starts with -, it must contain at least 3 characters.

@winstliu
Copy link
Contributor

winstliu commented Aug 5, 2016

My biggest issue with this is the use of lookbehinds.

@hediyi
Copy link
Contributor Author

hediyi commented Aug 6, 2016

Better? And *.scss -> *.css?

hediyi added 3 commits August 7, 2016 20:16
The animation-name can be specified as an identifier or string.

* A valid CSS identifier
  * begins with [-_A-Za-z], followed by any number of [-_A-Za-z0-9]
  * must contain at least 2 characters
  * if begins with -, the next character must be [_A-Za-z]

In fact, other characters are also allowed as long as they are escaped,
but escape sequences are rarely used in identifiers, and the inclusion
of which will result in a very complicated pattern.

* For keyframe selectors, values less than 0% or greater than 100% are
invalid.
@winstliu
Copy link
Contributor

winstliu commented Aug 7, 2016

.scss can be kept for now.

@hediyi hediyi changed the title Improve #at_rule_keyframes Improve the support for @keyframes Aug 7, 2016
@hediyi
Copy link
Contributor Author

hediyi commented Aug 7, 2016

Anything else?

@winstliu
Copy link
Contributor

winstliu commented Aug 7, 2016

Why is [[:blank:]] being used over \\s+?

@hediyi
Copy link
Contributor Author

hediyi commented Aug 8, 2016

TL;DR. Performance. But I just did a benchmark, turns out \s is slightly faster, bad decision. 😊 Changing it back to \s for brevity.

My thoughts:

\s is a shorthand for [\x20\t\v\f\n\r], [[:blank:]] is equivalent to [\x20\t].

  • \v and \f are basically never used in source code.
  • \n and \r are used in source code, but due to the way how a grammar package works—the regular expressions are matched against only a single line of the document at a time—something like [\n\r]@keyframes can never match since \n\r are always at the end of the previous line.

So in most cases, [[:blank:]] should be enough for tokenizing source code in Atom. I thought, by internal optimizations, [[:blank:]] and \s should be faster than [\x20\t], and [[:blank:]] may be faster than \s (I was WRONG).

@winstliu
Copy link
Contributor

winstliu commented Aug 8, 2016

Last thing: Replacing the space with \x20 seems to obfuscate things. Did you replace them because tends to get easily lost when scanning?

@hediyi
Copy link
Contributor Author

hediyi commented Aug 8, 2016

Still think \x20 is better than . 😆 See that "void" over there?

@hediyi hediyi closed this Aug 8, 2016
@hediyi hediyi reopened this Aug 8, 2016
@winstliu winstliu merged commit f7efe8f into atom:master Aug 8, 2016
@hediyi hediyi deleted the improve-keyframes branch August 10, 2016 12:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

comments inside keyframe declarations highlight incorrectly
2 participants