-
Notifications
You must be signed in to change notification settings - Fork 6
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: add search functionality to Topics view #1010
Conversation
3dba6f5
to
14d07ff
Compare
return `${this.subject} ${this.version}`; | ||
return this.id; |
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.
This mainly ensures we don't over-match while searching for a subject. It isn't currently possible to search for a schema ID in the Topics view since we don't have any eagerly-fetched Schema
s to attach to the subject container tree items, just the subjects themselves.
// @ts-expect-error we need to update @types/vscode to make ThemeColor.id public | ||
assert.strictEqual((icon as vscode.ThemeIcon).color!.id, "problemsWarningIcon.foreground"); |
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.
Unrelated to this PR: later versions of @types/vscode
provide id
: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/ab4715a8aead1731ced56619128c31e57bcc682d/types/vscode/index.d.ts#L904-L909
// set id to the label so it isn't `undefined`; can be overwritten by the caller if needed | ||
this.id = label.toString(); |
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.
This is done internally by VS Code as well, just not available to extensions
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/ab4715a8aead1731ced56619128c31e57bcc682d/types/vscode/index.d.ts#L11903-L11908
@@ -31,7 +31,7 @@ export const TEST_LOCAL_KAFKA_TOPIC: KafkaTopic = KafkaTopic.create({ | |||
name: "test-local-topic", | |||
partition_count: 1, | |||
clusterId: TEST_LOCAL_KAFKA_CLUSTER.id, | |||
environmentId: LOCAL_CONNECTION_ID, | |||
environmentId: TEST_LOCAL_KAFKA_CLUSTER.environmentId, |
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.
Unrelated to this PR: another EnvironmentId
branded type fix
Summary of Changes
Closes #1002.
The Topics view is now searchable with the same general mechanisms as the Resources view search done in #996, specifically to allow searching by topic name and schema subject.
topics-view-search.mov
Any additional details or context that should be provided?
Highlighting this part specifically:
vscode/src/loaders/loaderUtils.ts
Lines 85 to 91 in 8789c31
The search logic is fundamentally about what items are returned at each level of the tree data. The topics view is responsible for two main things upon loading at the root level: listing topics, and listing schema subjects to decorate the topics. Based on those two facts, the above snippet ensures we can search for a subject by preemptively setting a topic's
children
to be empty subject containers. If a Kafka topic matches based on its subject, it can then go through the normal motions of expanding to show the real subject container here:vscode/src/viewProviders/topics.ts
Lines 131 to 134 in 8789c31
...which we then are able to expand further and see all associated schema versions due to the subject container matching:
vscode/src/viewProviders/topics.ts
Lines 152 to 154 in 8789c31
All this to say: we can't search by any other pieces of information about a schema unless we eagerly load them and make sure they're available in the earlier-mentioned-empty-container-hack within
loaderUtils.ts
.Pull request checklist
Please check if your PR fulfills the following (if applicable):
Tests
Other
.vsix
file?