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

feat: display Kubernetes empty page when no current context #8989

Merged
merged 5 commits into from
Sep 25, 2024

Conversation

feloy
Copy link
Contributor

@feloy feloy commented Sep 20, 2024

Signed-off-by: Philippe Martin phmartin@redhat.com

What does this PR do?

Display Kubernetes empty page when no current context

Part of #8571, see #8571 (comment) for next steps

Screenshot / video of UI

kubernetes-empty-page.mp4

What issues does this PR fix or reference?

Part of #8571

There is a glitch on the Download button after the download is finished and the extension is removed from the list (the button of the next extension is displayed as Downloading for a second). Visible in the video around 0:16. The fix will be done as part of another PR

How to test this PR?

  • Tests are covering the bug fix or the new feature

@feloy feloy requested review from benoitf and a team as code owners September 20, 2024 07:10
@feloy feloy requested review from dgolovin, jeffmaury and gastoner and removed request for a team September 20, 2024 07:10
Copy link
Contributor

@gastoner gastoner left a comment

Choose a reason for hiding this comment

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

LGTM, tested on Fedora

@benoitf
Copy link
Collaborator

benoitf commented Sep 20, 2024

I like the iterations but IMHO it should be applied in some of reverse order.

  1. redirect people to the existing resources being installed (if no cluster then I should have a redirect to kind extension first)
    no need to install something else.

  2. enhance the embedded catalog to filter resources (keywords)

  3. enhance catalog of extensions with keywords for extensions that can bring a kubernetes cluster (locally or remotely)

  4. then include the catalog to this page (this PR) where people will see the page

IMHO the filtering is a pre-requisites before showing the link to the available extensions


On the page showed by the PR, I would expect maybe a note just on the top of the catalog explaining why it's displayed.

kind of sentence : Don't have a kubernetes cluster, install one these extensions to help you to bring one:

Signed-off-by: Philippe Martin <phmartin@redhat.com>
@feloy feloy force-pushed the feat-8571/kubernetes-empty-screen branch from 9a787a1 to cd462e0 Compare September 23, 2024 13:57
Copy link
Contributor

@jeffmaury jeffmaury left a comment

Choose a reason for hiding this comment

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

Not sure if it is to be reviewed as it is a draft

@feloy
Copy link
Contributor Author

feloy commented Sep 24, 2024

Not sure if it is to be reviewed as it is a draft

No, there is still work to do on this PR

Signed-off-by: Philippe Martin <phmartin@redhat.com>
@feloy feloy force-pushed the feat-8571/kubernetes-empty-screen branch from cd462e0 to 4bce304 Compare September 24, 2024 08:21
Signed-off-by: Philippe Martin <phmartin@redhat.com>
@feloy
Copy link
Contributor Author

feloy commented Sep 24, 2024

Not sure if it is to be reviewed as it is a draft

The features are now ready for review (still in Draft mode as I need to write some more unit tests)

Signed-off-by: Philippe Martin <phmartin@redhat.com>
@feloy feloy marked this pull request as ready for review September 24, 2024 12:35
Copy link
Contributor

@dgolovin dgolovin left a comment

Choose a reason for hiding this comment

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

LGTM + nits

packages/renderer/src/App.spec.ts Show resolved Hide resolved
packages/renderer/src/App.spec.ts Outdated Show resolved Hide resolved
packages/renderer/src/lib/kube/KubernetesEmptyPage.spec.ts Outdated Show resolved Hide resolved
Co-authored-by: Denis Golovin <dgolovin@users.noreply.github.com>
Signed-off-by: Philippe Martin <feloy1@gmail.com>
@feloy feloy force-pushed the feat-8571/kubernetes-empty-screen branch from 35cac5b to 70a1eb8 Compare September 25, 2024 07:21
Copy link
Contributor

@axel7083 axel7083 left a comment

Choose a reason for hiding this comment

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

First I want to say: Love it!

A small issue I encountered

Screenshot from 2024-09-25 09-58-18

I do not see minikube option, even tho I have it installed and enabled

image

I tried enabling/disabling it, same result.

@feloy
Copy link
Contributor Author

feloy commented Sep 25, 2024

First I want to say: Love it!

Thanks :)

A small issue I encountered

I do not see minikube option, even tho I have it installed and enabled

I tried enabling/disabling it, same result.

The logic should be the same as the "Create new..." button in the Resources page, for Minikube. Is the button visible there?

I have seen the same behaviour with another provider, where the button was not visible because a resource was already declared. Perhaps you already have a minikube cluster running?

Copy link
Collaborator

@benoitf benoitf left a comment

Choose a reason for hiding this comment

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

as a feedback, I think my eyes are more seeing the installation of other extensions than seeing the ability to already being able to create a cluster.

I don't know if we could balance more the upper part or add some divider between the top and bottom screen, or making more appealing the links to create a cluster

but it could be follow-up as it's a personal view.

@axel7083
Copy link
Contributor

The logic should be the same as the "Create new..." button in the Resources page, for Minikube. Is the button visible there?

Oh yeah, the minikube executable is not installed, therefore cannot create a cluster. That's a bit problematic, as when the user click for the first time to install the extension it would not allow him to directly create the cluster, since it would need to download the CLI first

Copy link
Contributor

@jeffmaury jeffmaury left a comment

Choose a reason for hiding this comment

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

LGTM.

In line with @benoitf comments, having more focus on Create would be nice

@feloy feloy merged commit 3ba8255 into podman-desktop:main Sep 25, 2024
15 checks passed
@podman-desktop-bot podman-desktop-bot added this to the 1.13.0 milestone Sep 25, 2024
@feloy
Copy link
Contributor Author

feloy commented Sep 25, 2024

as a feedback, I think my eyes are more seeing the installation of other extensions than seeing the ability to already being able to create a cluster.

I don't know if we could balance more the upper part or add some divider between the top and bottom screen, or making more appealing the links to create a cluster

but it could be follow-up as it's a personal view.

@ekidneyrh would you have some advice to make the first part more visible?

kubernetes-empty-page

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.

7 participants