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

(feat)Go to definition from class in template to css #1518

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

Jojoshua
Copy link
Contributor

@Jojoshua Jojoshua commented Jun 11, 2022

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
Copy link
Contributor Author

@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

@Jojoshua 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
Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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)

@@ -329,6 +330,31 @@ export class TypeScriptPlugin
const { lang, tsDoc } = await this.getLSAndTSDoc(document);
const mainFragment = tsDoc.getFragment();

const cssClassHelper = new CSSClassDefinitionLocator(tsDoc, position, document);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Feel free to relocate wherever you feel is best

@Jojoshua Jojoshua requested a review from dummdidumm June 16, 2022 11:51
@Jojoshua
Copy link
Contributor Author

Had a chance to look at again @dummdidumm?

@dummdidumm
Copy link
Member

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

@Jojoshua
Copy link
Contributor Author

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

@Jojoshua Jojoshua mentioned this pull request Aug 25, 2022
3 tasks
Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

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.

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

getCSSClassDefinition() {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

);

if (results) {
return results;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link

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
Copy link
Contributor Author

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

}
}

private getDefinitionRangeWithinStyleSection(targetClass: string) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants