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: details page for disk images #866

Merged
merged 2 commits into from
Sep 30, 2024
Merged

Conversation

deboer-tim
Copy link
Contributor

What does this PR do?

Modifies the build logs to have a proper details page for disk images, similar to how Podman Desktop handles Images or Containers.

The two tabs are Summary and Build Logs. The summary page is simple for now but could be expanded; the build logs doesn't really match what we typically have in Podman Desktop, but it fits in better here than a standalone page.

Screenshot / video of UI

Before:

Screen.Recording.2024-09-27.at.1.27.30.PM.mov

After:

Screen.Recording.2024-09-27.at.1.26.13.PM.mov

What issues does this PR fix or reference?

Fixes #856.

How to test this PR?

Build some disk images, switch between homepage and disk image, use homepage action to go straight to a build.

Modifies the build logs to have a proper details page for disk images, similar
to how Podman Desktop handles Images or Containers.

The two tabs are Summary and Build Logs. The summary page is simple for now
but could be expanded; the build logs doesn't really match what we typically
have in Podman Desktop, but it fits in better here than a standalone page.

Fixes podman-desktop#856.

Signed-off-by: Tim deBoer <[email protected]>
@deboer-tim deboer-tim requested a review from a team as a code owner September 27, 2024 17:31
@deboer-tim deboer-tim requested review from cdrage, feloy and SoniaSandler and removed request for a team September 27, 2024 17:31
Comment on lines 21 to 22
console.log('id: ' + actualId);
console.log('hist: ' + historyInfo.subscribe.length);

Choose a reason for hiding this comment

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

Suggested change
console.log('id: ' + actualId);
console.log('hist: ' + historyInfo.subscribe.length);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

console.log('hist: ' + historyInfo.subscribe.length);
return historyInfo.subscribe(value => {
const matchingImage = value.find(image => image.id === actualId);
console.log('match: ' + matchingImage?.id);

Choose a reason for hiding this comment

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

Suggested change
console.log('match: ' + matchingImage?.id);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

@@ -0,0 +1,30 @@
/**********************************************************************
* Copyright (C) 2023-2024 Red Hat, Inc.

Choose a reason for hiding this comment

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

Suggested change
* Copyright (C) 2023-2024 Red Hat, Inc.
* Copyright (C) 2024 Red Hat, Inc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/upstream is where we've been copying code as-is from PD (until hopefully one day it is part of the UI library), but I've updated this header.


export function isTabSelected(routerPath: string, tabUrl: string): boolean {
return routerPath === getTabUrl(routerPath, tabUrl);
}

Choose a reason for hiding this comment

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

Some unit tests maybe ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied this from PD, where there were no tests 😬. I've added some, and will PR to PD as well.

Copy link

@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.

Tested locally on Windows 11 and works like a charm 🚀 LGTM (some comments added)

{#if image}
<tr>
<Cell>Source image</Cell>
<Cell>{image.image}:{image.tag}</Cell>

Choose a reason for hiding this comment

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

Something nice (follow up PR) could be done here, is to have the image as a link and redirect to the image details page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! Yes, I'll do this via another PR.

Removed unnecessary console.log debugging and added tests for Util.

Signed-off-by: Tim deBoer <[email protected]>
Copy link
Contributor

@cdrage cdrage left a comment

Choose a reason for hiding this comment

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

Looks absolutely great and thanks for this change!

After testing, the implementation looks great and I have no issues with the code / code LGTM.

I'm going to rebase #865 so that it's in a tab similar to this implementation after this is merged in.

@cdrage cdrage merged commit 7b17a94 into podman-desktop:main Sep 30, 2024
6 checks passed
deboer-tim added a commit to deboer-tim/desktop that referenced this pull request Sep 30, 2024
As part of podman-desktop/extension-bootc#866
it was noticed that the Util class is missing tests for a couple functions,
this just adds them.

Signed-off-by: Tim deBoer <[email protected]>
cdrage pushed a commit to podman-desktop/podman-desktop that referenced this pull request Sep 30, 2024
As part of podman-desktop/extension-bootc#866
it was noticed that the Util class is missing tests for a couple functions,
this just adds them.

Signed-off-by: Tim deBoer <[email protected]>
@deboer-tim deboer-tim deleted the details branch October 11, 2024 18:12
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.

Add a Disk Image details page
3 participants