-
Notifications
You must be signed in to change notification settings - Fork 56
Added support for render tag completion for inline snippets #1068
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
base: add-doc-tag-snippet-support
Are you sure you want to change the base?
Added support for render tag completion for inline snippets #1068
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
1cabd73 to
bcee95e
Compare
a47fc50 to
391578c
Compare
bcee95e to
692492c
Compare
692492c to
3e5b8a1
Compare
3e5b8a1 to
599e439
Compare
2b6b68c to
984c187
Compare
599e439 to
1da6cbe
Compare
984c187 to
80f1e20
Compare
1da6cbe to
122e6f8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments, but overall looks great! 🥳
...es/theme-language-server-common/src/completions/providers/RenderSnippetCompletionProvider.ts
Outdated
Show resolved
Hide resolved
...es/theme-language-server-common/src/completions/providers/RenderSnippetCompletionProvider.ts
Outdated
Show resolved
Hide resolved
...es/theme-language-server-common/src/completions/providers/RenderSnippetCompletionProvider.ts
Outdated
Show resolved
Hide resolved
1751d79 to
4467ee0
Compare
80f1e20 to
61e1110
Compare
5b21535 to
e008de5
Compare
61e1110 to
61afe9c
Compare
5f6220f to
cef145f
Compare
24dbcf0 to
6465a20
Compare
50eaabe to
aa24f09
Compare
6465a20 to
0263556
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| } | ||
| } | ||
|
|
||
| function getInlineSnippetsNames(ast: LiquidHtmlNode): string[] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oooh yeah you're right! Thanks for catching that 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is tricky, hi-snip could be passed in via render to hi-snip-2, but should tooling expect that? Maybe we could just limit completion to prevent a snippet being offered inside itself and leave the rest as is?
@aswamy do you think that would make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could just limit completion to prevent a snippet being offered inside itself and leave the rest as is?
I think you're right. For simplicity, we should only offer code completion to things that are explicitly defined within the snippet.
This includes ALL variables. The reason is that we aren't sure how the snippet is rendered, which can get very confusing.
0263556 to
9366de6
Compare
85ae0e5 to
ba844e6
Compare
9366de6 to
b4049b2
Compare
3886acb to
59eb787
Compare
59eb787 to
791ddf5
Compare




What are you adding in this PR?
Solves #827
Added completion support for inline snippets in the render tag. Now when using the
{% render %}tag, the language server will suggest both file-based snippets and inline snippets defined within the same file. Inline snippets are displayed with a helpful documentation note indicating they are "defined in this file".What's next? Any followup issues?
changeset