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

Add support for custom properties #88

Merged
merged 4 commits into from
Sep 15, 2016
Merged

Add support for custom properties #88

merged 4 commits into from
Sep 15, 2016

Conversation

jiayihu
Copy link
Contributor

@jiayihu jiayihu commented Sep 14, 2016

This is an attempt to start improving the support for CSSNext: #51
It's also my first time with a language package so please tell me if I did something wrong.

@@ -483,6 +483,10 @@
'include': '#comment-block'
}
{
'match': '--([A-Za-z0-9_-]+)'
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs a more complex pattern in order to avoid matching ------ as a variable, which I'm pretty sure is invalid. @esdoppio can probably give some tips regarding this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I improved it with --([A-Za-z][A-Za-z0-9_-]*), but I'm open to suggestions.

Copy link
Contributor

Choose a reason for hiding this comment

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

"------" is indeed a really lousy name, but it's valid (test it yourself), so I think we should allow it (to be tokenized) and let people make their own decisions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Huh. I stand corrected.

@winstliu
Copy link
Contributor

  • var(--variable-name) support needs to be added.
  • Is the :root pseudo-selector matched?
  • Specs are required.

@jiayihu
Copy link
Contributor Author

jiayihu commented Sep 14, 2016

  • var(--variable-name) support was already added by someone else
  • The :root pseudo selector is matched as any other pseudo-selector. Its scope is indeed source.css, meta.selector.css, entity.other.attribute-name.pseudo-class.css
  • I added the specs. First time writing in CS 😅

I noticed that the specs for var(--variable-name) are probably missing, should I add them also? Also the pattern for var(--variable) matches var(------) too as my previous RegExp, so I believe it should be fixed too.

@winstliu
Copy link
Contributor

I noticed that the specs for var(--variable-name) are probably missing, should I add them also? Also the pattern for var(--variable) matches var(------) too as my previous RegExp, so I believe it should be fixed too.

If you could do both of these, that would be ✨

@@ -483,7 +483,8 @@
'include': '#comment-block'
}
{
'match': '--([A-Za-z0-9_-]+)'
'match': '--([A-Za-z][A-Za-z0-9_-]*)'
Copy link
Contributor

Choose a reason for hiding this comment

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

The pattern is a bit too strict.

From https://www.w3.org/TR/css-variables/#custom-property:

it’s defined as any valid identifier that starts with two dashes

And a valid identifier shares the same syntax as an <ident-token>—(escaping set aside) it

  • may contain only
    • [a-zA-Z0-9_-]
    • ISO 10646 characters U+00A0 and higher
  • must not begin with [0-9]
  • must not begin with '-' followed by [0-9]
  • must be at least 2 characters long if begins with '-'

Since the identifier in this case always starts with --, the last three requirements are fulfilled, you only have to care about the first rule (in bold) here. So a simple

--[\\w-]+

suffices. Just in case, \w is not equivalent to [a-zA-Z0-9_] with the Unicode defined encodings like UTF-8. 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, even

--[\\w-]+

is too strict regarding the syntax, but since no one is really complaining about they want to use emoji or escaping in, like, class names, I think we're good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@esdoppio Thank you so much for your help ☺️

@jiayihu
Copy link
Contributor Author

jiayihu commented Sep 15, 2016

Okay so...

  • Updated the Regexp for both custom properties and custom variables
  • Added spec for both
  • Squashed some commits in order to have a cleaner history

Anything else that I missed? 🤔

@winstliu winstliu merged commit 7ebfdf9 into atom:master Sep 15, 2016
@winstliu
Copy link
Contributor

Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants