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.

Sign up for GitHub

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

Jump to bottom

GH-3985: Support for css/html via VS Code extension #7972

Merged
merged 2 commits into from
Jun 11, 2020
Merged

GH-3985: Support for css/html via VS Code extension #7972

merged 2 commits into from
Jun 11, 2020

Conversation

kittaakos
Copy link
Contributor

@kittaakos kittaakos commented Jun 5, 2020 •
edited by akosyakov
Loading

What it does

Removed the CSS/HTML integration from monaco, and uses the corresponding VS Code extensions instead.

How to test

VS Code built-in HTML support:

  • diagnostics,
    • Check that you can produce and invalid HTML by defining a completely invalid script. Try <script>totoal...invalid = $Hello JavaScript!$;</script>, you should see the error in the Problems view too. You can also check the diagnostics for invalid HTML: <html><script></scaript></html>.
      Screen Shot 2020-06-08 at 13 44 08
  • snippet support, proposes completion for the '.', ':', '<', '“', '=', and '/' chars,
  • completion,
    • Open an HTML file type < see the tag completions.
      Screen Shot 2020-06-08 at 13 34 03
  • hover,
    • Open /Users/akos.kitta/git/theia/packages/plugin-ext/src/main/browser/webview/pre/index.html hover over name in meta tag, see the hover.
      Screen Shot 2020-06-08 at 13 33 18
  • document symbols,
    • Verify you see document symbols for the current HTML document (Ctrl/Cmd+Shift+O)
      Screen Shot 2020-06-08 at 13 57 41
  • document highlight,
    • In a HTML file write the followings: <div><div></div></div>, select the inner div, verify that the only corresponding closing div is highlighted, not all of them.
      Screen Shot 2020-06-08 at 13 37 52
  • definitions,
    • You can use a script tag if you want to verify the definitions, check both Go to Definition and Peek > Peek Definition. Both should work. Here is an example: <script>const x = 'foo'; console.log(x);</script>.
      Screen Shot 2020-06-08 at 14 16 07
  • signature helper (triggered by '('),
    • Verify the signature helper, create a script tag, add a function, invoke it
      Screen Shot 2020-06-08 at 14 07 27
  • references,
    • Create a script tag and verify the JS references. Similar to defintions.
  • range folding,
    • Verify that you can expand/collapse ranges.
      screencast 2020-06-08 13-54-37
  • range formatting.
    • Make sure you can format a subset of the document. Open an HTML, select an unformatted range, format is. Formatting the selection will require both 'textDocument/foldingRange' and 'textDocument/rangeFormatting'. You can enable tracing for HTML via the settings.
      screencast 2020-06-08 13-48-10
  • selection range,
    • See range formatting 👆
  • rename, and
    • Verify rename. Rename a tag to something.
      screencast 2020-06-08 14-09-14
  • semantic highlighting.
    • No idea how to verify this.

VS Code built-in CSS/SCSS/Less support:

  • diagnostics,
    • Check that you can generate errors in an invalid CSS file, the corresponding error shows up in the Problems view. Try {{}} in the editor and see the error: at-rule or selector expected file:///Users/akos.kitta/git/theia/a.css. Check for warnings, write p > div { } without rulesets, see the Do not use empty rulesets warning in the Problems view. You can disable validation by adding "css.validate": false to your settings.json.
  • completion,
    • You can see the default #region and #endregion completions in a CSS file. Also, type p > div { d<|> } (where <|> is the cursor position) you should see display: grid|inline; completion.
  • snippet support, proposes completion for the '/' and '-' chars,
  • hover,
    • Check that hover works, the easiest is by hovering over an url and opening the file in an editor. For example: --theia-debug-start: url('start-inverse.svg').
  • document symbol,
    • Open a non-empty CSS file, press Ctrl/Cmd+Shift+O you can see the selectors defined in the CSS file.
  • references,
    • Right-click on a selector, you can trigger Find All References, the Refrences view opens and shows your selector.
  • definitions,
    • It's only partially supported for CSS. Open packages/core/src/browser/style/menus.css, right-click on the --theia-private-menubar-height usesage in the .p-MenuBar-item selector, pick Go to Definition from the context menu.
      screencast 2020-06-08 12-11-30
  • document highlight,
    • Open tree.css in Theia, set the cursor after the first occurrence of .theia-Tree in the CSS file, see that other occurrences are highlighted in the file.
  • code actions,
    • Although code actions are not defined for CSS, you should see the enabled Source Actions... context menu item (with the browser example) from a CSS editor. Executing it will result in a No source actions available in the editor.
  • rename,
    • You can rename a selector in a CSS file. Open tree.css, for example, and rename .theia-Tree with the context menu (or F2). You can rename the symbol in the file. It won't update other occurrences outside of the current CSS file.
  • range folding,
    • Sorround one of the CSS selectors with /* #region */ and /* #endregion */. Add "[css]": { "editor.foldingStrategy": "indentation" } into the settings.json file, see in the editor, you can expand and fold ranges
      screencast 2020-06-08 11-59-25
  • document link, and
    • You can open a url('someLink.css') in another editor.
  • selection range.
    • I have no idea how to test this.

Review checklist

Reminder for reviewers

Akos Kitta added 2 commits June 5, 2020 11:55
GH-3985: Support for css/tml via VS Code extension
3bf7245 
Closes #3985

Signed-off-by: Akos Kitta <kittaakos@typefox.io>
Removed a leftover await.
3debc5d 
Signed-off-by: Akos Kitta <kittaakos@typefox.io>
@akosyakov akosyakov added monaco issues related to monaco vscode issues related to VSCode compatibility labels Jun 5, 2020
@kittaakos
Copy link
Contributor Author

kittaakos commented Jun 8, 2020 •
edited
Loading

Issues:

  • Open an empty css files, try the content assist:
errors.ts:26 Uncaught Error: Cannot read property 'Deprecated' of undefined

TypeError: Cannot read property 'Deprecated' of undefined
   at y (/Users/akos.kitta/gi…t/dist/cssMain.js:1)
   at /Users/akos.kitta/gi…t/dist/cssMain.js:1
   at C (/Users/akos.kitta/gi…t/dist/cssMain.js:1)
   at Array.map (<anonymous>)
   at asCompletionResult (/Users/akos.kitta/gi…t/dist/cssMain.js:1)
   at y (/Users/akos.kitta/gi…t/dist/cssMain.js:1)
   at /Users/akos.kitta/gi…t/dist/cssMain.js:1
   at C (/Users/akos.kitta/gi…t/dist/cssMain.js:1)
   at Array.map (<anonymous>)
   at asCompletionResult (/Users/akos.kitta/gi…t/dist/cssMain.js:1)
   at errors.ts:26
  • Open a HTML file:
root INFO [hosted-plugin: 10818] PLUGIN_HOST(10818): PluginManagerExtImpl/loadPlugin(/Users/akos.kitta/git/theia/plugins/vscode-builtin-npm/extension/dist/main)
root ERROR [hosted-plugin: 10818] Promise rejection not handled in one second: TypeError: s.SemanticTokensLegend is not a constructor , reason: TypeError: s.SemanticTokensLegend is not a constructor
root ERROR [hosted-plugin: 10818] With stack trace: TypeError: s.SemanticTokensLegend is not a constructor
   at b.sendRequest.then.e (/Users/akos.kitta/git/theia/plugins/vscode-builtin-html-language-features/extension/client/dist/htmlMain.js:1:114062)

@kittaakos kittaakos marked this pull request as ready for review June 8, 2020 12:17
@kittaakos kittaakos requested a review from akosyakov June 8, 2020 12:17
@akosyakov
Copy link
Member

@kittaakos Does #7972 (comment) break user flow or it is just error logging?

@kittaakos
Copy link
Contributor Author

Does #7972 (comment) break user flow or it is just error logging?

I did not notice malfunction during the verification, the errors were "just" logged. I have seen the same s.SemanticTokensLegend is not a constructor error for TS earlier. I assume, the same API is missing or what.

@akosyakov
Copy link
Member

I did not notice malfunction during the verification, the errors were "just" logged. I have seen the same s.SemanticTokensLegend is not a constructor error for TS earlier. I assume, the same API is missing or what.

👍 An issue for the first error would be good.

@kittaakos
Copy link
Contributor Author

👍 An issue for the first error would be good.

Sure, let's merge this, and then I take care of the follow-up.

@akosyakov
Copy link
Member

@marcdumais-work @vince-fugnitto Could you review it please? I am stuck with FS api rewrite. This PR is much needed to move in the direction of getting rid of monaco-languageclient.

vince-fugnitto
vince-fugnitto approved these changes Jun 9, 2020
View reviewed changes
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

I verified the changes based on the 'how to test' and everything works as intended thank you!
I noticed the following notification when first opening a html file but this is expected (the features still worked correctly):

Screenshot from 2020-06-09 15-08-59

@akosyakov
Copy link
Member

akosyakov commented Jun 10, 2020 •
edited
Loading

I noticed the following notification when first opening a html file but this is expected

I don't think it is expected. Could you file an follow-up issue please? Are there any errors in logs? We have to fix it.

@kittaakos
Copy link
Contributor Author

I don't think it is expected. Could you file an follow-up issue please? Are there any errors in logs? We have to fix it.

I will take care of any follow-ups, once this is merged.

@kittaakos
Copy link
Contributor Author

Are there any additional remarks on this PR, or can I merge the changes? Right after the merge, I am going to create follow-up issues.

@vince-fugnitto vince-fugnitto mentioned this pull request Jun 10, 2020
Closed
@vince-fugnitto
Copy link
Member

I noticed the following notification when first opening a html file but this is expected

I don't think it is expected. Could you file an follow-up issue please? Are there any errors in logs? We have to fix it.

I opened the follow-up issue: #7987

@vince-fugnitto vince-fugnitto mentioned this pull request Jun 11, 2020
Closed
@kittaakos kittaakos merged commit daede30 into master Jun 11, 2020
@kittaakos kittaakos deleted the GH-3985 branch June 11, 2020 08:34
@azatsarynnyy azatsarynnyy mentioned this pull request Jul 6, 2020
Merged
This was referenced Dec 9, 2020
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vince-fugnitto vince-fugnitto vince-fugnitto approved these changes

@akosyakov akosyakov Awaiting requested review from akosyakov

@marcdumais-work marcdumais-work Awaiting requested review from marcdumais-work

Assignees
No one assigned
Labels
monaco issues related to monaco vscode issues related to VSCode compatibility
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kittaakos @akosyakov @vince-fugnitto