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

Added Components Metadata #161 #186

Merged
merged 5 commits into from
Jul 1, 2021
Merged

Conversation

sk593
Copy link
Contributor

@sk593 sk593 commented Jun 21, 2021

Added component metadata (name of component) to the application tree view. this addresses one aspect of issue #161 and addresses #167 that requests to expose more metadata. An example of the components view is shown below

Screen Shot 2021-06-22 at 11 02 34 AM

@philliphoff
Copy link
Collaborator

@sk593 Can you add a screenshot showing the new UX; that often helps with reviews.

@sk593 sk593 reopened this Jun 23, 2021
@sk593
Copy link
Contributor Author

sk593 commented Jun 23, 2021

I initially pushed changes to address the comments but it caused some Linux package checks to fail. I accidentally uncommitted those changes when trying to fix the error so the original PR doesn't have the initial commits I made. But the last commit should have all the original changes, as well as the requested updates @philliphoff

@sk593 sk593 changed the title Addded Components Metadata #161 Added Components Metadata #161 Jun 25, 2021


export default class DaprComponentsNode implements TreeNode {
constructor(public readonly metadata: string, public readonly application: DaprApplication) {
constructor(public readonly application: DaprApplication, public readonly daprClient: DaprClient) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: unless they have to be accessible outside of the class, use private readonly.

if(components.length > 0) {
return components.map(comp => new DaprMetadataNode(comp.name, 'database'));
}
return [new DaprMetadataNode(label, 'warning')];
}

private async getMetadata(application: DaprApplication, token?: vscode.CancellationToken | undefined): Promise<DaprMetadata> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: as this is now only a single line function, used from a single location, is it necessary?

@@ -5,15 +5,15 @@ import * as vscode from 'vscode';
import TreeNode from "../treeNode";

export default class DaprMetadataNode implements TreeNode {
constructor(public readonly metadata: string) {
constructor(public readonly metadata: string, public readonly icon: string) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps themeIconId: string to reinforce its usage (as opposed to a resource path string, which is another way to specify an icon)?

@philliphoff
Copy link
Collaborator

@sk593 A couple of final nits, but otherwise looks good. Let's also make sure there is an issue filed (maybe assigned to @AaronCrawfis) to get a better set of icons into the product at some point.

@philliphoff philliphoff merged commit 6aea135 into microsoft:main Jul 1, 2021
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.

2 participants