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: provider cards #5013

Merged
merged 5 commits into from Dec 13, 2023
Merged

feat: provider cards #5013

merged 5 commits into from Dec 13, 2023

Conversation

deboer-tim
Copy link
Collaborator

What does this PR do?

Creates a ProviderCard component and makes each of the Provider* components use it in order to get some common layout and code between the different states.

With the card I tried to make it more like the example in #4395 and our other cards (e.g. Resources, Kubernetes contexts, or CLI), creating two columns with the first having the common bits: logo at the top-left corner with the name and version, with status below it, and the right containing everything else that is specific to the state.

The existing status dots + text components don't quite work for providers so I created a new one. Need to review this, overall componentization, and create tests for ProviderCard before raising for official review.

Screenshot/screencast of this PR

Before:

Screenshot 2023-11-27 at 4 16 14 PM

After:

Screenshot 2023-11-28 at 12 54 14 AM

What issues does this PR fix or reference?

Fixes most of #4395. IMHO we should still remove a link or two from providers, and the horizontal Podman/Docker logos should be changed to regular square logos like the other providers.

How to test this PR?

yarn test:renderer and view Dashboard for any UI issues.

@mairin
Copy link
Member

mairin commented Dec 2, 2023

The docs / resources links are kinda awkward (they were in the mockup too!) I am wondering, can we put them under a kebab menu in the upper right corner of the card for each?

@mairin
Copy link
Member

mairin commented Dec 2, 2023

Also for podman - can we just drop to the podman logomark? We don't need the logotype (this is the same approach as on settings > resources)

@mairin
Copy link
Member

mairin commented Dec 2, 2023

Ahhh one more thing. Sorry. We should have a link to settings > resources. One idea would be to put these cards in an encapsulating box and put a manage ... button attached to the upper right of the encapsulating box... and that would go to settings > resources. something like this (I worked in the kebabs too so you could see how it might look):

Screenshot from 2023-12-01 21-33-44

@deboer-tim
Copy link
Collaborator Author

I've opened #5099 to switch to using the square logos, since it is independent to this PR.

This PR is really just to get a common component/layout between the cards. I can see if some of the new design is easy to incorporate, but the rest should go in following PRs. The new design doesn't match what's in issue #4395 though, so we should either update that or create another issue so this design is not lost.

FWIW I like the cleanliness and simplicity, but there is still other information (e.g. see Podman and OpenShift Local above) where we need to decide where else to put the actions or if they can be dropped. There is also no 'reconnect' action or link icons today.

@mairin
Copy link
Member

mairin commented Dec 4, 2023

@deboer-tim cool I can put this new mockup there - apologies for this, i wasn't quite happy with the original mockup.

re: the extra functionality - i have this issue:
#4284

the thought is:

  • if there's functionality (eg init podman machine, init crc, update crc) that is on the dashboard that isn't in settings > resources, it should be moved to settings > resources.
  • if the provider is in a state that needs user attention (eg podman machine not initialized, crc not initialized, crc has an update pending) those should be notification cards instead and/or pop up as notifications in the task manager.

@benoitf
Copy link
Collaborator

benoitf commented Dec 5, 2023

can be follow-up PRs, etc but probably need to land after 1.6 is cut

Creates a ProviderCard component and makes each of the Provider* components use
it in order to get some common layout and code between the different states.

With the card I tried to make it more like the example in #4395 and our other
cards (e.g. Resources, Kubernetes contexts, or CLI), creating two columns with
the first having the common bits: logo at the top-left corner with the name and
version, with status below it, and the right containing everything else that is
specific to the state.

The existing status dots + text components don't quite work for providers so I
created a new one. Need to review this, overall componentization, and create
tests for ProviderCard before raising for official review.

Signed-off-by: Tim deBoer <git@tdeboer.ca>
Signed-off-by: Tim deBoer <git@tdeboer.ca>
@deboer-tim deboer-tim marked this pull request as ready for review December 12, 2023 03:49
@deboer-tim deboer-tim requested review from benoitf and a team as code owners December 12, 2023 03:49
@deboer-tim deboer-tim requested review from cdrage and feloy and removed request for a team December 12, 2023 03:49
@deboer-tim
Copy link
Collaborator Author

Rebased, force pushed, and fixed missing aria region to fix e2e tests.

Ready for official review. To reiterate, this is not implementing the full nor final design: it is moving everything to a single component and doing the most basic part of the design change, to make it easier and cleaner for future PRs under #4395 to continue the process.

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.

ProviderCard has no tests

Basic tests for aria region, name, version/no-version, and state added.

Signed-off-by: Tim deBoer <git@tdeboer.ca>
Signed-off-by: Tim deBoer <git@tdeboer.ca>
@benoitf benoitf merged commit 2bf9ab2 into main Dec 13, 2023
8 checks passed
@benoitf benoitf deleted the provider-card branch December 13, 2023 17:42
@podman-desktop-bot podman-desktop-bot added this to the 1.7.0 milestone Dec 13, 2023
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