-
Notifications
You must be signed in to change notification settings - Fork 514
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
Add group by for images #823
Conversation
Awesome!!!!! |
@cmeyertons :-) |
@EricJizbaMSFT Any chance you can look at this today? Thx. |
Yeah I can take a look. First impression is that I might prefer if this experience was more similar to filtering subscriptions. Aka instead of context menus, it would be:
|
I think I like that. Of course I already implemented #3. |
Probably need to do this as a separate PR. |
extension.ts
Outdated
@@ -169,6 +170,9 @@ export async function activateInternal(ctx: vscode.ExtensionContext, perfStats: | |||
); | |||
registerDebugConfigurationProvider(ctx); | |||
|
|||
let imageGrouping: number | undefined = ext.context.globalState.get<ImageGrouping>(groupImagesByKey); |
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.
I would prefer if this was a normal setting instead of in the globalState.
- More discoverable
- Let's me control the setting at a per-workspace level if desired
- Let's me more easily share settings with others (by sending someone settings json or by committing settings.json to git)
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 falls under what I would consider temporary (local) user settings. Don't see much sense in sharing this, for example, and you're likely to switch between different settings in a session. Doesn't seem like scc should be invoked on that. Discoverability is not an issue since it's listed right in the images node title and the menus. Also the exact groupings may still be a bit experimental.
I could see perhaps having a default grouping in settings, but don't want to deal with that now for similar reasons.
cc @fiveisprime
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.
Don't see much sense in sharing this
Even if I'm not sharing with others, I would definitely share with myself - aka copy/paste my settings.json when I create a new VM.
you're likely to switch between different settings in a session
Why would I be likely to switch? When I'm given multiple options for how to display UI, I generally pick my favorite and keep it there.
Also the exact groupings may still be a bit experimental.
If the groupings change, it'll affect globablState the same as it would affect settings. Besides if you're REALLY worried the groupings will change, I would argue the feature isn't ready to be released yet anyways.
I agree this is somewhat low on the "value" graph, but it seems trivial to implement as a setting and I just don't see any downsides.
extension.ts
Outdated
@@ -264,6 +268,11 @@ function registerDockerCommands(): void { | |||
dockerExplorerProvider | |||
); | |||
|
|||
registerCommand('vscode-docker.images.cycleGroupBy', cycleGroupImagesBy); |
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 I don't like cycleGroupBy. I would expect this button to pop up a quick pick
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.
Okay, not sure I agree, thought you were suggesting getting rid of the explicit menu options.
@fiveisprime What do you think?
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.
I guess I don't have a string opinion, but since Matt already reviewed the current version, let's have him decide (after the PR).
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.
thought you were suggesting getting rid of the explicit menu options.
I'm not a fan of the explicit menu options, but I'm okay leaving them in there if you're worried about discoverability. I think the inline button with a quick pick should be the main entry point.
Using a cycle like this requires me to click multiple times (even more times as you add different groupings) just to get to the grouping I want. Seems like a quick pick is faster and shows me all options at once
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.
I feel somewhat strongly on these, but as always if you file an issue to address (or at least discuss) later I will approve as-is
extension.ts
Outdated
@@ -169,6 +170,9 @@ export async function activateInternal(ctx: vscode.ExtensionContext, perfStats: | |||
); | |||
registerDebugConfigurationProvider(ctx); | |||
|
|||
let imageGrouping: number | undefined = ext.context.globalState.get<ImageGrouping>(groupImagesByKey); |
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.
Don't see much sense in sharing this
Even if I'm not sharing with others, I would definitely share with myself - aka copy/paste my settings.json when I create a new VM.
you're likely to switch between different settings in a session
Why would I be likely to switch? When I'm given multiple options for how to display UI, I generally pick my favorite and keep it there.
Also the exact groupings may still be a bit experimental.
If the groupings change, it'll affect globablState the same as it would affect settings. Besides if you're REALLY worried the groupings will change, I would argue the feature isn't ready to be released yet anyways.
I agree this is somewhat low on the "value" graph, but it seems trivial to implement as a setting and I just don't see any downsides.
extension.ts
Outdated
@@ -264,6 +268,11 @@ function registerDockerCommands(): void { | |||
dockerExplorerProvider | |||
); | |||
|
|||
registerCommand('vscode-docker.images.cycleGroupBy', cycleGroupImagesBy); |
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.
thought you were suggesting getting rid of the explicit menu options.
I'm not a fan of the explicit menu options, but I'm okay leaving them in there if you're worried about discoverability. I think the inline button with a quick pick should be the main entry point.
Using a cycle like this requires me to click multiple times (even more times as you add different groupings) just to get to the grouping I want. Seems like a quick pick is faster and shows me all options at once
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.
Thanks :)
In spite of my comments :-), thanks for the input. |
Fixes #603
cc @cmeyertons