(feat)Go to definition from class in template to css by Jojoshua · Pull Request #1518 · sveltejs/language-tools

Jojoshua · 2022-06-11T19:04:22Z

Implementation help for go to css definition from template in #83

Works for many different class formats:

Standard
- class="test test1"
Conditional Expression
- class="{current === 'baz' ? 'selected' : ''
Class Directive
- class:active="{current === 'foo'}"
HTML Element Tag

<main>

<style>
 main {
 ...
 }
</style>

Jojoshua · 2022-06-11T19:11:29Z

@dummdidumm Side-note #83 regarding go to definition from css to template, I don't feel this is a "go to definition" concern but rather a find references because the class can be used many places within the template

dummdidumm

Thank you for tackling this! The idea is good, I left some comments on the implementation, where code should go in my opinion and what a more robust class lookup might look like

packages/language-server/src/plugins/css/CSSClassDefinitionLocator.ts

dummdidumm · 2022-06-14T18:25:23Z

packages/language-server/src/plugins/css/CSSClassDefinitionLocator.ts

+
+    public IsExactClassMatch(testRange: Range) {
+        //Check nothing before the test position
+        if (testRange.start.character > 0) {


This would fail for something like .foo.bar { .. } or .foo .bar { .. }. In general it's a little brittle to only use text matching here. The vscode-css-language-service which is used in the CSSPlugin provides an AST I think, but it's private. This puts us at risk of breaking changes but I feel it's more robust to use that one instead and fall back to this text-based matching only if parsing fails (because a different preprocessor is used for example).

I had a feeling about that one. The case I thought would be the most straightforward turned out, not. I will try to improve it. I was looking for a way to pull the text for an entire line but couldn't find it

Note that svelte doesn't recognize the .foo .bar case either

@dummdidumm So this actually did already work for .foo .bar but did not for .foo.bar

Try the latest, it handles all sorts of combinations. I feel this is a good compromise especially since I really don't feel that style's inside a svelte component would/should be very complex. If after testing you still feel like we need more horsepower I can look into the CSS AST(just point me in the right direction for that)

dummdidumm · 2022-06-14T18:30:50Z

packages/language-server/src/plugins/typescript/TypeScriptPlugin.ts

        const { lang, tsDoc } = await this.getLSAndTSDoc(document);
        const mainFragment = tsDoc.getFragment();

+        const cssClassHelper = new CSSClassDefinitionLocator(tsDoc, position, document);


The idea of the plugin folders is that they are mostly independent of each other, so it would be better to put this logic into the CSSPlugin or move the class into the TypeScript plugin folder - but I feel like the best place would be SveltePlugin.ts. This brings us to a question to which I don't have a good answer yet: What is the best place for code like this? It's cross-cutting so it's not obvious to me in which plugin this does belong. Maybe this hints at that the plugin system needs a redesign / needs to be replaced with something new, but that's for another day.

Yeah I was having trouble where to place this. I moved to TypeScript folder for now.

Feel free to relocate wherever you feel is best

Jojoshua · 2022-06-23T01:37:52Z

Had a chance to look at again @dummdidumm?

dummdidumm · 2022-06-25T18:12:04Z

Sorry, have some other things on my plate currently, but I'll get back to this in the next weeks!

Jojoshua · 2022-08-25T01:18:17Z

@dummdidumm Just checking in -- didn't want to lose too much context on this as time passed

dummdidumm

Sorry for leaving you hanging for so long, my primary focus is on SvelteKit right now. I left two comments from what I think could be improved, hope this helps.

dummdidumm · 2022-08-26T13:02:26Z

packages/language-server/src/plugins/typescript/CSSClassDefinitionLocator.ts

+        this.initialNodeAt = this.tsDoc.svelteNodeAt(this.position) as SvelteNode;
+    }
+
+    getCSSClassDefinition() {


I believe it could be more promising/faster to defer CSS parsing to vscode-css-languageservice which has a parse method. The AST that is returned doesn't have type definitions because it's private, but I'm honestly not worried too much about that since it's unlikely to change for this. That AST could be traversed to more easily find the correct node to link to.

Looking into this again. Using svelteNodeAt is giving back a lot of good Svelte information that I wouldn't think the vscode-css-languageservice would know about. This method will tell us all the other variations of Svelte css such as whether it is a ConditionalExpression or Class Directive Format.

I didn't mean the HTML part, I meant the CSS part. The code does string traversal of the CSS to find the matching definition, but it could be more promising to use the CSS AST output of vscode-css-languageservice

Are you referring to the while loop in getStandardFormatClassName() ? This is actually not traversing css but the class attribute string itself to figure out which classname the position is at

class="test te|st1 bar foo" so in this example the cursor is in the middle of test1 so that's what we're going to look for in the style section

dummdidumm · 2022-08-26T13:03:52Z

packages/language-server/src/plugins/typescript/TypeScriptPlugin.ts

+            );
+
+            if (results) {
+                return results;


The results should probably be merged with what else comes up. In the case of class:foo both .foo { ... } in <style> and let foo = ... in script are definitons, and we would lose the latter when returning early.

I'm not sure I follow. In all the tests and variations I've used it seems to work. When you hit F12 to go to definition there can only be 1 result (if a definition can be found).

lukaszpolowczyk · 2022-09-02T11:34:56Z

Definitely this function should also take into account the name of the element (tagName)
As far as the proposal doesn't take it into account.

Just referring to classes is not enough.

Also the recognition of fixed attributes can be taken into account.

Jojoshua · 2022-12-21T19:01:40Z

Definitely this function should also take into account the name of the element (tagName) As far as the proposal doesn't take it into account.

Just referring to classes is not enough.

Also the recognition of fixed attributes can be taken into account.

Done via 6010236

dummdidumm · 2023-01-09T11:00:52Z

packages/language-server/src/plugins/typescript/CSSClassDefinitionLocator.ts

+        }
+    }
+
+    private getDefinitionRangeWithinStyleSection(targetClass: string) {


Apologies, the comment on line getCSSClassDefinition was actually meant to appear here. That code does a somewhat unperformant string search. I believe it could be more promising/faster to defer CSS parsing to vscode-css-languageservice which has a parse method. The AST that is returned doesn't have type definitions because it's private, but I'm honestly not worried too much about that since it's unlikely to change for this. That AST could be traversed to more easily find the correct node to link to.

Initial

c22d449

Jojoshua changed the title ~~Go to definition from class in template to css~~ (feat)Go to definition from class in template to css Jun 11, 2022

Jojoshua added 3 commits June 11, 2022 15:40

refactor

6e42c84

refactor

65920ea

Add tests

bd2b9c4

dummdidumm requested changes Jun 14, 2022

View reviewed changes

Jojoshua added 2 commits June 14, 2022 19:03

cleanup

b01e66c

Improve css combination termination cases

7f86d5c

Jojoshua requested a review from dummdidumm June 16, 2022 11:51

Jojoshua mentioned this pull request Aug 25, 2022

Go To Definition for CSS #169

Open

3 tasks

dummdidumm reviewed Aug 26, 2022

View reviewed changes

Jojoshua added 2 commits December 21, 2022 00:29

Merge branch 'master' into feat/83/GoToCSSDefinition

c87212b

Add support for navigating to element tagname

6010236

Jojoshua requested a review from dummdidumm December 21, 2022 19:04

dummdidumm reviewed Jan 9, 2023

View reviewed changes

Uh oh!

Conversation

Jojoshua commented Jun 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jojoshua commented Jun 11, 2022

Uh oh!

dummdidumm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jojoshua commented Jun 23, 2022

Uh oh!

dummdidumm commented Jun 25, 2022

Uh oh!

Jojoshua commented Aug 25, 2022

Uh oh!

dummdidumm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukaszpolowczyk commented Sep 2, 2022

Uh oh!

Jojoshua commented Dec 21, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Jojoshua commented Jun 11, 2022 •

edited

Loading