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

Support content_for block type completion + document link #709

Merged
merged 1 commit into from
Jan 17, 2025

Conversation

aswamy
Copy link
Contributor

@aswamy aswamy commented Jan 14, 2025

What are you adding in this PR?

Fixes #571

Support content_for block type completion + document link

Before you deploy

  • I included a minor bump changeset
  • My feature is backward compatible

@aswamy aswamy requested a review from charlespwd January 14, 2025 22:08
@aswamy aswamy requested a review from a team as a code owner January 14, 2025 22:08
Copy link
Contributor

@karreiro karreiro 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 this PR, @aswamy! Excellent stuff 🎩 It works as expected, and I've left only one minor comment about the completion items :)


return (await this.getThemeBlockNames(doc.uri, false)).map((blockName) => ({
label: blockName,
kind: CompletionItemKind.Keyword,
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't seem like we're completing a keyword, so I personally believe we should use CompletionItemKind.EnumMember, as the completion items are part of a well-defined set of options (standardized and formally grouped).

Alternatively, we could use CompletionItemKind.Value as well, but it doesn't seem so ideal since it refers to literal values not tied to a certain group.

(Just mentioning this because this impacts the icons of the completion items, so it also gives extra clues for users) :)

Copy link
Contributor

@charlespwd charlespwd left a comment

Choose a reason for hiding this comment

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

LGTM!

@aswamy aswamy force-pushed the feature/content_for-block-type-completion branch from b16e3ec to 2cba55f Compare January 15, 2025 21:58
@aswamy aswamy merged commit ccc0c95 into main Jan 17, 2025
6 checks passed
@aswamy aswamy deleted the feature/content_for-block-type-completion branch January 17, 2025 17:17
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.

Support content_for "block" type completion + document link
3 participants