Skip to content
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

Add CSS, GraphQL, and SQL template literal tags to Tree-Sitter grammar #638

Merged
merged 5 commits into from Feb 7, 2019

Conversation

Projects
None yet
3 participants
@bennypowers
Copy link
Contributor

bennypowers commented Jan 25, 2019

Description of the Change

Similar to the html, gql, or sql template literal tags which are already supported, this PR adds support for a css template literal tag.

My only concern with the PR is with my use of string.quoted.template.css.js on line 1384. Does this need to be defined elsewhere?

Alternate Designs

I think the feature is pretty straightforward, the alternative is not to have the CSS grammar in such templates.

Benefits

Use cases like the following will receive CSS grammars:

import { LitElement, css } from 'lit-element';
class MyEl extends LitElement {
  static get styles()  {
    return css`
      :host {
        display: block;
      }
    `;
  }
}

Possible Drawbacks

Added maintenance burden.

Applicable Issues

@bennypowers bennypowers changed the title Add `css` template literal tag WIP: Add `css` template literal tag Jan 25, 2019

@bennypowers bennypowers force-pushed the bennypowers:css-tagged-templates branch 6 times, most recently from 297c90d to 04eb090 Jan 25, 2019

@bennypowers

This comment has been minimized.

Copy link
Contributor Author

bennypowers commented Jan 25, 2019

So, I think the tests are well-formed, but I get results like:

Expected { value : '${display', scopes : [ 'source.js', 'string.quoted.template.css.js', 'meta.property-list.css', 'meta.property-value.css' ] } to equal { value : 'display', scopes : [ 'source.js', 'string.quoted.template.css.js', 'source.js.embedded.source' ] }.

Note the value: ${display which indicates to me that this token isn't being picked up as a js interpolation, despite line 1390: 'include': '#interpolated_js'

Not sure how to proceed from here.

@bennypowers bennypowers force-pushed the bennypowers:css-tagged-templates branch from 04eb090 to 4f905c5 Jan 25, 2019

@bennypowers

This comment has been minimized.

Copy link
Contributor Author

bennypowers commented Jan 25, 2019

It's worth noting that this works out of the box when switching grammars to typescript.

In fact, if I switch grammars to typescript, then switch back to javascript, I still have highlighting in my template.

@Aerijo

This comment has been minimized.

Copy link
Member

Aerijo commented Jan 25, 2019

In fact, if I switch grammars to typescript, then switch back to javascript, I still have highlighting in my template.

@bennypowers Switching grammars like that is actually reverting you to the old TextMate grammar. See atom/atom#18738

@daviwil

This comment has been minimized.

Copy link
Member

daviwil commented Feb 7, 2019

Hi @bennypowers, since we've moved to using Tree-Sitter for language-javascript we would prefer it if you added your change to that grammar configuration instead. Turns out that this is very easy, you just need to add a check for 'css' in this function's if statement and return CSS. No tests are required as we aren't currently testing Tree-Sitter grammar mappings in this repo.

If you're interested in updating your PR to take this approach, I'll be happy to merge it and include it in the next update. Thanks!

Revert "Add `css` template literal tag"
This reverts commit 4fc67fb98d66c239f639bf33171ec261c59e4c72.
@bennypowers

This comment has been minimized.

Copy link
Contributor Author

bennypowers commented Feb 7, 2019

K cool

If this is good, I'm happy to rebase

Show resolved Hide resolved lib/main.js Outdated

@bennypowers bennypowers force-pushed the bennypowers:css-tagged-templates branch from 20b3c73 to 14f2863 Feb 7, 2019

@bennypowers

This comment has been minimized.

Copy link
Contributor Author

bennypowers commented Feb 7, 2019

should we include gql and sql here?

@daviwil

This comment has been minimized.

Copy link
Member

daviwil commented Feb 7, 2019

Sure, just update the PR title to include those so that it's clear when they got added.

Show resolved Hide resolved lib/main.js Outdated

@bennypowers bennypowers changed the title WIP: Add `css` template literal tag WIP: Add CSS, GraphQL, and SQL template literal tags Feb 7, 2019

@bennypowers

This comment has been minimized.

Copy link
Contributor Author

bennypowers commented Feb 7, 2019

Please check language string on line 74

@daviwil daviwil changed the title WIP: Add CSS, GraphQL, and SQL template literal tags Add CSS, GraphQL, and SQL template literal tags to Tree-Sitter grammar Feb 7, 2019

@daviwil

This comment has been minimized.

Copy link
Member

daviwil commented Feb 7, 2019

Looks great, thanks a lot @bennypowers!

@daviwil daviwil merged commit d95c26b into atom:master Feb 7, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.