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

path completion for css. fix #45235 #45788

Merged
merged 4 commits into from
Mar 20, 2018
Merged

path completion for css. fix #45235 #45788

merged 4 commits into from
Mar 20, 2018

Conversation

octref
Copy link
Contributor

@octref octref commented Mar 14, 2018

To go together with microsoft/vscode-css-languageservice#76

Questions:

  • How can we avoid duplication of code between html/css? (utils, path completion logic / test)
  • Should we even expose methods such as onCssURILiteralValue in CSS Lang Service? I imagine this would be no use to anything else other than path completion. Since one of March's task is simplify consumption of our language servers, I feel adding all these API surface area is going to make the consumption of LS harder.

Copy link
Contributor

@aeschli aeschli left a comment

Choose a reason for hiding this comment

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

Please also add tests...

@@ -0,0 +1,79 @@
/*---------------------------------------------------------------------------------------------
Copy link
Contributor

Choose a reason for hiding this comment

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

only copy the functions you need

@aeschli
Copy link
Contributor

aeschli commented Mar 14, 2018

How can we avoid duplication of code between html/css

  • once we merge the html / css server this could be solved
  • the alternative is to have the common functionality in a node module

@aeschli
Copy link
Contributor

aeschli commented Mar 14, 2018

Should we even expose methods such as onCssURILiteralValue

Yes, agree, the more API we have, the harder it is to maintain this. Happy to go with any proposal that requires less API. Alternative could be to give the CSS service a fileSystemContext that returns folders and files. Then the logic of creating the proposals would stay in the service. (but would still be duplicated in both the HTML/CSS language services)

@octref octref changed the title [WIP] path completion for css. fix #45235 path completion for css. fix #45235 Mar 19, 2018
import { TextDocument, TextEdit } from 'vscode-languageserver-types';
import { mergeSort } from './arrays';

export function applyEdits(document: TextDocument, edits: TextEdit[]): string {
Copy link
Contributor

Choose a reason for hiding this comment

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

The latest vscode-languageserver-types now offers
TextDocument.applyEdits

items: []
};
cssLS.setCompletionParticipants([getPathCompletionParticipant(document, workspaceFolders, pathCompletionList)]);
const result = getLanguageService(document).doComplete(document, textDocumentPosition.position, stylesheets.get(document))!; /* TODO: remove ! once LS has null annotations */
Copy link
Contributor

Choose a reason for hiding this comment

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

use variable 'cssLS'

{ label: 'about/', resultText: `html { background-image: url('../about/')` },
{ label: 'index.html', resultText: `html { background-image: url('../index.html')` },
{ label: 'src/', resultText: `html { background-image: url('../src/')` }
]
Copy link
Contributor

Choose a reason for hiding this comment

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

Please also test:
url('../a|')
url('../a|b')
url(a|)

@octref octref merged commit 54f1a18 into master Mar 20, 2018
@joaomoreno joaomoreno deleted the octref/pathCompletion branch April 30, 2018 08:31
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
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.

2 participants