microsoft / typescript-lit-html-plugin

TypeScript server plugin that adds intellisense for lit-html template strings
MIT License
255 stars 22 forks source link

Add basic CSS IntelliSense #9

Closed justinribeiro closed 6 years ago

justinribeiro commented 6 years ago

This PR adds what I would consider a basic level of support for CSS intellisense within html and raw (#8 and https://github.com/mjbvz/vscode-lit-html/issues/29)

  1. Completion of CSS properties works properly.
  2. Completion of CSS property values works properly.
  3. Everything else previously works the same (HTML and Typescript/JS var completion).

What this PR doesn't do at the moment:

  1. CSS error diagnostics for mistyped props/values (which I think will take a bit more effort to get right and should be done separately)
  2. Doesn't scope the CSS completion list to <style> (I could think of some lit-html approaches that may not have <style>, so it's pretty loose here in an attempt to make the developer experience better on the whole). Fixed in 0ecb6c3

I'm happy to make changes if this isn't the proper approach or if y'all would like this a different way. I couldn't find a lot of best practices for the approach to this (though I did look through Microsoft/typescript-template-language-service-decorator to try to get a grasp).

image image

msftclas commented 6 years ago

CLA assistant check
All CLA requirements met.

justinribeiro commented 6 years ago

That small commit should resolve the documentation problems that were failing on the CI test (tests now pass locally).

image image

mjbvz commented 6 years ago

Cool. I'm a little concerned that this may start showing css completions outside of style tags though (or that previous html content may be break the css intellisense). Do you see either of these things happening?

Here's what the vscode html extension uses to determine if a request was made inside css or not: https://github.com/Microsoft/vscode/blob/master/extensions/html-language-features/server/src/modes/embeddedSupport.ts

justinribeiro commented 6 years ago

I have similar trepidation around css completions outside of style; in my limited testing today, it appeared as less of a problem than I expected honestly. The only case that was guaranteed to trigger CSS outside of style was empty line start. Didn't see any css completion on < or inside of a tag.

I'll have a look at the embeddedSupport.ts you linked to tomorrow morning and do some more testing and get you a more definitive answer. Appreciate the help!

justinribeiro commented 6 years ago

@mjbvz Having looked over the embeddedSupport.ts, I understand what the lang server does. Since I can't import the embedded support from the server, I forked and parred down the lang check and then specifically check what language is at the current position, returning only the proper list.

This results in a significantly better experience; no duplicates from css in the html list and vice versa. In the example screenshot below, you can see we only have html completion, none duplicated from the css list:

image

mjbvz commented 6 years ago

Nice! Excited to have this merged in.

Can you please just add a few tests for regressions and to check that the new functionality works as expected. The unit tests currently only run on node 7 but I should be able to to push a small fix for this within the next few hours

justinribeiro commented 6 years ago

Will do. I'll get some tests added and pushed this afternoon.

justinribeiro commented 6 years ago

The tests I added test a few things:

  1. Tests CSS completions
  2. Tests to make sure that CSS/HTML completions do not cross-pollute
  3. Tests the embeddedSupport to make sure that language identification works properly (these tests were derived from the existing html-lang-server (https://github.com/Microsoft/vscode/blob/master/extensions/html-language-features/server/src/test/embedded.test.ts))

Everything in this PR should pass the expanded test suite without issue.

image

mjbvz commented 6 years ago

Thanks! I'll do a quick test pass and we'll get this published if nothing major comes up

mjbvz commented 6 years ago

Published with 0.3.0. Will pick up in the lit-html vs code plugin too