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

Fix #110: Recognize keyframes at-rule with vendor prefixes#111

Merged
winstliu merged 1 commit intoatom:masterfrom
clchen28:keyframes-vendor-prefix
Apr 4, 2017
Merged

Fix #110: Recognize keyframes at-rule with vendor prefixes#111
winstliu merged 1 commit intoatom:masterfrom
clchen28:keyframes-vendor-prefix

Conversation

@clchen28
Copy link
Copy Markdown
Contributor

@clchen28 clchen28 commented Apr 2, 2017

Description of the Change

Added -webkit-keyframes, -moz-keyframes, -o-keyframes, and -ms-keyframes to begin and patterns regex for @keyframes in grammar.cson, allowing these to follow the same syntax highlighting rules as @keyframes.

Fix #110

Benefits

Applies @keyframes at-rule syntax highlighting correctly if vendor prefixes are used

Applicable Issues

#99 Add support for literally every modern CSS feature

@winstliu
Copy link
Copy Markdown
Contributor

winstliu commented Apr 2, 2017

Your change can be simplified to (-(webkit|moz|o)-)?keyframes. I'll leave it to @Alhadis to decide if the vendor prefix should receive additional scopes.

@clchen28
Copy link
Copy Markdown
Contributor Author

clchen28 commented Apr 2, 2017

Ok - amended previous commit with simplified regex (-(webkit|moz|o)-)?keyframes.

@Alhadis
Copy link
Copy Markdown
Contributor

Alhadis commented Apr 3, 2017

Nope, no need for additional scopes. I'd make the groups non-capturing, though, to save a little extra overhead.

@clchen28
Copy link
Copy Markdown
Contributor Author

clchen28 commented Apr 3, 2017

Ok @Alhadis - I've amended the commit and updated the groups to be non-capturing,

e.g.
'begin': '(?i)(?=@(?:(?:-(?:webkit|moz|o|ms)-)?keyframes)([\\s\'"{;]|/\\*|$))'

and

'begin': '(?i)\\G(@)(?:(?:-(?:webkit|moz|o|ms)-)?keyframes)'

@Alhadis
Copy link
Copy Markdown
Contributor

Alhadis commented Apr 3, 2017

@50Wliu LGTM.

@Ben3eeE Ben3eeE requested a review from winstliu April 3, 2017 06:58
{
# @keyframes
'begin': '(?i)(?=@keyframes([\\s\'"{;]|/\\*|$))'
'begin': '(?i)(?=@(?:(?:-(?:webkit|moz|o|ms)-)?keyframes)([\\s\'"{;]|/\\*|$))'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The outside capture can be removed, so this can be simplified to be (?:-(?:webkit|moz|o|ms)-)?keyframes. Same for below.

(?i)(?=@(?:-(?:webkit|moz|o|ms)-)?keyframes([\\s\'"{;]|/\\*|$))

@clchen28
Copy link
Copy Markdown
Contributor Author

clchen28 commented Apr 4, 2017

Ok - @50Wliu per your review, I've amended the commit to remove the outer capture for the regex, e.g. (?i)(?=@(?:-(?:webkit|moz|o|ms)-)?keyframes([\\s\'"{;]|/\\*|$)) and (?i)\\G(@)(?:-(?:webkit|moz|o|ms)-)?keyframes

@winstliu winstliu merged commit 5a4f277 into atom:master Apr 4, 2017
@winstliu
Copy link
Copy Markdown
Contributor

winstliu commented Apr 4, 2017

Thanks!

@clchen28
Copy link
Copy Markdown
Contributor Author

clchen28 commented Apr 5, 2017

:D My first ever PR for FOSS! Thanks for the review.

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.

Incorrect syntax highlighting for keyframes at-rule when using vendor prefixes

3 participants