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

fix: show current context in quick pick #2920

Merged
merged 2 commits into from Jun 19, 2023

Conversation

deboer-tim
Copy link
Collaborator

What does this PR do?

Even when the quick pick is fixed to show which items are pre-selected (issue #2754 item 2) it is not really clear to the user why one row is highlighted. This adds the text " (current)" to the current Kube context to make this obvious.

Screenshot/screencast of this PR

Screenshot 2023-06-19 at 9 01 59 AM

What issues does this PR fix or reference?

N/A

How to test this PR?

Click on the context status bar to see the quick pick.

Even when the quick pick is fixed to show which items are pre-selected (issue containers#2754 item 2) it is
not really clear to the user why one row is highlighted. This adds the text " (current)" to the
current Kube context to make this obvious.

Signed-off-by: Tim deBoer <git@tdeboer.ca>
@deboer-tim deboer-tim requested review from a team and benoitf as code owners June 19, 2023 13:02
@deboer-tim deboer-tim requested review from jeffmaury and lstocchi and removed request for a team June 19, 2023 13:02
@benoitf
Copy link
Collaborator

benoitf commented Jun 19, 2023

shouldn't we fix the quickpick component instead ?

@deboer-tim
Copy link
Collaborator Author

We should fix the quick pick too, but to me it's still not obvious why one item is selected by default. e.g. in the image if kind-kind-cluster-5 was selected I still need to think for a second to realize that's because it's the current context. I'm not sure how else to show this, especially for new users.

@benoitf
Copy link
Collaborator

benoitf commented Jun 19, 2023

@deboer-tim should we put the current item as the first item of the list ?

@benoitf
Copy link
Collaborator

benoitf commented Jun 19, 2023

I mean, the extension could always set as current value the one being selected (independent of the quickpick)

@@ -110,7 +110,7 @@ export async function updateContext(

quickPicks = contexts.map(context => {
return {
label: context.name,
label: context.name === currentContext ? context.name + ' (current)' : context.name,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
label: context.name === currentContext ? context.name + ' (current)' : context.name,
label: context.name === currentContext ? `${context.name} (current)` : context.name,

@jeffmaury
Copy link
Contributor

I think the current context should be the one selected by default and VSCode OpenShift uses another styling:

image

Jeff's suggestion is better.

Signed-off-by: Tim deBoer <git@tdeboer.ca>
@deboer-tim
Copy link
Collaborator Author

Quickpicks can have a description?? 😀

Switched to use the same styling, using a current context description is better than changing the label:
Screenshot 2023-06-19 at 11 44 07 AM

@deboer-tim deboer-tim merged commit f0a0743 into containers:main Jun 19, 2023
8 checks passed
@podman-desktop-bot podman-desktop-bot added this to the 1.2.0 milestone Jun 19, 2023
@deboer-tim deboer-tim deleted the context2 branch June 26, 2023 13:15
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.

None yet

4 participants