feat: add examples page #942

cdrage · 2024-10-17T19:40:42Z

feat: add examples page

What does this PR do?

Minor edit to category (see json update)
Adds an example page (rendered portion) that shows all the examples we
currently have
Pull and build image buttons
Reactively do in-progress for pulling

Screenshot / video of UI

Screen.Recording.2024-10-17.at.3.32.42.PM.mov

What issues does this PR fix or reference?

Closes #895

How to test this PR?

Go to examples page
Press pull image button
Then build image button after it finishes

Signed-off-by: Charlie Drage [email protected]

cdrage · 2024-10-17T19:43:04Z

Tests have been added! Ready for testing out and using 🚀

I have added some "follow up issues" to the topic here, especially lack of examples (only 3 "good" ones right now): #864 (comment)

benoitf · 2024-10-17T21:45:27Z

packages/frontend/src/lib/ExamplesCard.svelte

+      const example = examples.find(example => example.image === msg.image);
+      if (example) {
+        example.state = 'pulled';
+        console.log(examples);


to be removed

benoitf · 2024-10-17T21:46:04Z

packages/frontend/src/lib/ExamplesCard.svelte

+// Function to update examples based on available images
+function updateExamplesWithPulledImages() {
+  if (bootcAvailableImages) {
+    bootcAvailableImages.forEach(image => {


I think the linter is now recommending. for of pattern

benoitf · 2024-10-17T21:47:01Z

packages/frontend/src/lib/ExamplesCard.svelte

+    bootcAvailableImages.forEach(image => {
+      // Only do it if there is a RepoTags
+      if (image.RepoTags) {
+        const [imageRepo, imageTag] = image.RepoTags[0].split(':');


maybe use the optional ? pattern to reduce the if if if chaining

benoitf · 2024-10-17T21:47:36Z

packages/frontend/src/lib/ExamplesCard.svelte

+}
+
+// Reactive statement to call the function each time bootcAvailableImages updates
+$: if (bootcAvailableImages) {


might be a time to use $effect / svelte 5 runes

Can't as we use $$props.class in the Card (copied from AI lab and we try to keep consistent with them too) which we can then not use since it puts the svelte file in "runes mode"

~~NVM, we don't actually use classes imported into it, so it's okay to switch to svelte 5 runes, updating now.~~

Could not get it to work reactively very well, even using $props as well as $effects. May just leave this like it is for now.

I don't see any reason why it would fail

benoitf · 2024-10-17T21:47:55Z

packages/frontend/src/lib/ExamplesCard.svelte

+<Card title={category.name} classes="{$$props.class} font-medium mt-4">
+  <div slot="content" class="w-full">
+    {#if examples.length === 0}
+      <div class="text-gray-400 mt-2">There is no example in this category.</div>


should ot use empty screen ?

No need :) will always have examples!

Tried your suggestion though using empty screen but messed up formatting, so may be easier to just use this.

todo: if we always have examples, then do not add the if clause

benoitf · 2024-10-17T21:49:46Z

packages/frontend/src/lib/ExampleCard.spec.ts

+import { render, fireEvent, screen } from '@testing-library/svelte';
+import { expect, test, vi } from 'vitest';
+import ExampleCard from './ExampleCard.svelte';
+import { bootcClient } from '../api/client';


use absolute import

packages/frontend/src/lib/Card.svelte

benoitf · 2024-10-17T21:51:08Z

packages/frontend/src/lib/Card.svelte

+</script>
+
+<a class="no-underline" href={href}>
+  <div class="{classes} rounded-md flex-nowrap overflow-hidden" role="region" aria-label={title ?? 'Card'}>


I think usually we're using another pattern than having an explicit classes parameter

We are doing this in a few places, often with different parameter names. In Svelte 5 you can do:

let { class: className = '', }: { class?: string; } = $props();

to make this cleaner, but since we ever only ever do classes="font-medium mt-4", maybe remove the parameter and put the styling in here?

packages/frontend/src/Examples.svelte

benoitf · 2024-10-17T21:52:46Z

packages/backend/assets/examples.json

@@ -38,7 +38,7 @@
  "categories": [
    {
      "id": "fedora",
-      "name": "Fedora BootC Images"
+      "name": "Fedora Bootable Container Images"


could be a separate small PR

deboer-tim

Ditto to all the points @benoitf raised.

Worked fine when testing. The biggest gap to me was how to find details or instructions for each example. Why does it exist, what does it do, and how do I use it? The source goes to the repo root, then I have to guess at the folder and look at the readme. (and ok, Tailscale doesn't have anything)

Could we link directly to the folder, or ideally have a way to view the readme directly? Maybe needs a details page for each one?

Noted a few other minor usability issues:

The page appears, then we switch buttons from Pull to Build. Is the API fast enough that we could do this before opening the page, or can we delay adding the button until we know which it is? (might need to move Source button to Link on the left so it doesn't move).
Like on the dashboard, you don't know the image you're about to pull is huge. Would be nice to include the size on the card, or warn the user when they click on the button.
No pull task or progress. Unrelated to this issue so I opened up Usability of pulling images #944.

cdrage · 2024-10-21T13:54:12Z

Ditto to all the points @benoitf raised.

Worked fine when testing. The biggest gap to me was how to find details or instructions for each example. Why does it exist, what does it do, and how do I use it? The source goes to the repo root, then I have to guess at the folder and look at the readme. (and ok, Tailscale doesn't have anything)

Could we link directly to the folder, or ideally have a way to view the readme directly? Maybe needs a details page for each one?

Noted a few other minor usability issues:

The page appears, then we switch buttons from Pull to Build. Is the API fast enough that we could do this before opening the page, or can we delay adding the button until we know which it is? (might need to move Source button to Link on the left so it doesn't move).

Like on the dashboard, you don't know the image you're about to pull is huge. Would be nice to include the size on the card, or warn the user when they click on the button.

No pull task or progress. Unrelated to this issue so I opened up Usability of pulling images #944.

We have https://gitlab.com/fedora/bootc/examples/-/issues/19 which I could work on next sprint with regards to descriptions of what each one does: #947

We have no way of knowing the size of the image before pulling, but we can at least add an approximate "size" on the description when pulling?

ACK on button delay / showing it correctly.

cdrage · 2024-10-21T15:00:02Z

@deboer-tim @benoitf

Updated the code based on the review:

Added size of the image (hardcoded in examples.json), makes it easier and less calls to check for size (honestly sizes will not change much, and this is just for showing examples)
Modified how we show the buttons on load, less flickering / changes.
Did not update to svelte5 (could not get it to work very well without causing extra changes in other svelte files)
Updated rest of minor code edits

UI update (shows size now):

deboer-tim · 2024-10-21T21:04:11Z

I'm no longer getting Pull buttons when I don't have the image locally:

cdrage · 2024-10-22T14:22:09Z

I'm no longer getting Pull buttons when I don't have the image locally:

that's my mistake, I had messed up an if statement.

Latest commit I have also updated the styling, adding a "querying" button in case it takes a while to get the list of images, etc, as well as reduce the amount of flickering on initial load.

deboer-tim

Thanks for the changes, worked well in testing. We have some ideas for additional improvements open, but this delivers on the core requirement.

Some minor requests for change but otherwise this is good to go for me.

deboer-tim · 2024-10-22T18:07:56Z

packages/frontend/src/lib/Card.svelte

+</script>
+
+<a class="no-underline" href={href}>
+  <div class="{classes} rounded-md flex-nowrap overflow-hidden" role="region" aria-label={title ?? 'Card'}>


We are doing this in a few places, often with different parameter names. In Svelte 5 you can do:

let { class: className = '', }: { class?: string; } = $props();

to make this cleaner, but since we ever only ever do classes="font-medium mt-4", maybe remove the parameter and put the styling in here?

deboer-tim · 2024-10-22T18:08:52Z

packages/frontend/src/lib/ExampleCards.spec.ts

+import { render, screen } from '@testing-library/svelte';
+import { expect, test, vi } from 'vitest';
+import type { Example, Category } from '/@shared/src/models/examples';
+import ExamplesCard from './ExamplesCard.svelte';


component: ExamplesCard, test: ExampleCards. Can you rename one so that they match?

deboer-tim · 2024-10-22T18:10:30Z

packages/frontend/src/Examples.svelte

@@ -0,0 +1,56 @@
+<script lang="ts">
+import type { Example, Category } from '../../shared/src/models/examples';


Should really use /@shared/... here, and other places.

cdrage · 2024-10-22T20:04:02Z

@deboer-tim @benoitf Updated based on the review comments and ready for another review 👍

deboer-tim

Thanks for the changes @cdrage, a nice little addition we can continue to expand on.

benoitf · 2024-10-23T15:03:55Z

packages/frontend/src/Examples.spec.ts

+
+  // Wait until example1 says Build image as it updates reactively
+  // same for example 2 but Pull image
+  waitFor(() => {


issue: missing await before waitFor

benoitf · 2024-10-23T15:04:01Z

packages/frontend/src/Examples.spec.ts

+    expect(buildImage1).toBeInTheDocument();
+  });
+
+  waitFor(() => {


issue: missing await before waitFor

benoitf · 2024-10-23T15:06:28Z

packages/frontend/src/lib/Card.svelte

+export let title: string | undefined = undefined;
+export let description: string | undefined = undefined;
+
+export let href: string | undefined = undefined;


suggestion: use svelte5 runes

Suggested change

export let title: string | undefined = undefined;

export let description: string | undefined = undefined;

export let href: string | undefined = undefined;

interface Props {

title: string;

description: string;

href: string

}

let {title, description, href} : Props = $props();

benoitf · 2024-10-23T15:06:37Z

packages/frontend/src/lib/Card.svelte

+export let href: string | undefined = undefined;
+</script>
+
+<a class="no-underline" href={href}>


Suggested change

<a class="no-underline" href={href}>

<a class="no-underline" {href}>

benoitf · 2024-10-23T15:08:20Z

packages/frontend/src/lib/ExampleCard.svelte

+// Function that takes something in MB and returns it in GB is more than 1GB
+// in a nice string format
+function formatStringSize(size: number): string {
+  if (size > 1000) {
+    return `${(size / 1000).toFixed(1)} GB`;
+  }
+  return `${size} MB`;
+}


todo: please use humansize library

https://www.npmjs.com/package/human-size

I decided to use the filesize library https://www.npmjs.com/package/filesize which is used in podman desktop. the one you linked hasn't been updated in 10 years.

ah yes I think I took the wrong name but you figured it out
I mixed humanize-duration and this one

benoitf · 2024-10-23T15:09:18Z

packages/frontend/src/lib/ExampleCard.svelte

+$: {
+  example.state;
+}


issue: feels hacky

use svelte5 ($props, $effect, etc)

benoitf · 2024-10-23T15:10:15Z

packages/frontend/src/lib/ExampleCard.svelte

+          icon={faArrowDown}
+          aria-label="Pull image"
+          title="Pull image"
+          class="w-28"


question: do we need to set a size there ?

benoitf · 2024-10-23T15:10:30Z

packages/frontend/src/lib/ExampleCard.svelte

+          inProgress={pullInProgress}>Pull image</Button>
+      {:else}
+        <!-- Show a spinner / in progress for querying button instead if we are still loading information-->
+        <Button icon={faArrowDown} aria-label="Querying" title="Querying" class="w-28" inProgress={true}


question: same here, why do we need to set a size there ? (w-28)

benoitf · 2024-10-23T15:11:18Z

packages/frontend/src/lib/ExamplesCard.svelte

+}
+
+// Reactive statement to call the function each time bootcAvailableImages updates
+$: if (bootcAvailableImages) {


I don't see any reason why it would fail

benoitf · 2024-10-23T15:11:51Z

packages/frontend/src/lib/ExamplesCard.svelte

+<Card title={category.name} classes="{$$props.class} font-medium mt-4">
+  <div slot="content" class="w-full">
+    {#if examples.length === 0}
+      <div class="text-gray-400 mt-2">There is no example in this category.</div>


todo: if we always have examples, then do not add the if clause

### What does this PR do? * Minor edit to category (see json update) * Adds an example page (rendered portion) that shows all the examples we currently have * Pull and build image buttons * Reactively do in-progress for pulling ### Screenshot / video of UI  ### What issues does this PR fix or reference?  Closes podman-desktop#895 ### How to test this PR?  1. Go to examples page 2. Press pull image button 3. Then build image button after it finishes Signed-off-by: Charlie Drage <[email protected]>

Signed-off-by: Charlie Drage <[email protected]>

cdrage · 2024-10-23T19:15:44Z

Updated @benoitf @benoitf

The more I use svelte5, the more I'm liking it! Much cleaner code.

I have updated it to svelte5 paradigms. I also decided to use https://www.npmjs.com/package/filesize library instead of humansize since it's the same on in Podman Desktop + is more updated.

Functionality of it working:

Screen.Recording.2024-10-23.at.3.17.35.PM.mov

Signed-off-by: Charlie Drage <[email protected]>

benoitf · 2024-10-23T19:19:01Z

yes the library name was wrongly spelled !

benoitf · 2024-10-23T19:21:24Z

looks like there are some typechecking issue but approving

cdrage · 2024-10-23T19:39:53Z

looks like there are some typechecking issue but approving

Thanks! Formatter had messed up and did not put {} around href. Added and updated now.

cdrage requested a review from a team as a code owner October 17, 2024 19:40

cdrage requested review from dgolovin, jeffmaury and deboer-tim and removed request for a team October 17, 2024 19:40

benoitf reviewed Oct 17, 2024

View reviewed changes

deboer-tim reviewed Oct 18, 2024

View reviewed changes

cdrage force-pushed the add-examples-page branch 4 times, most recently from b1c339d to 9cef29a Compare October 21, 2024 14:59

cdrage force-pushed the add-examples-page branch 2 times, most recently from d4a109a to 9a3da6f Compare October 21, 2024 20:45

cdrage force-pushed the add-examples-page branch 3 times, most recently from fffee57 to 8db094c Compare October 22, 2024 14:20

deboer-tim added this to the 1.5 milestone Oct 22, 2024

cdrage requested a review from benoitf October 22, 2024 16:10

deboer-tim reviewed Oct 22, 2024

View reviewed changes

cdrage force-pushed the add-examples-page branch from a8eecb1 to 5647d96 Compare October 22, 2024 20:12

deboer-tim approved these changes Oct 23, 2024

View reviewed changes

benoitf reviewed Oct 23, 2024

View reviewed changes

cdrage added 2 commits October 23, 2024 13:52

update based on review

ac382f7

Signed-off-by: Charlie Drage <[email protected]>

cdrage added 2 commits October 23, 2024 13:52

update size

0450b10

Signed-off-by: Charlie Drage <[email protected]>

update based on review

ab95e69

Signed-off-by: Charlie Drage <[email protected]>

cdrage force-pushed the add-examples-page branch 2 times, most recently from ec715a7 to ab53d85 Compare October 23, 2024 19:14

use svelte5, update to code review

2e21ee2

Signed-off-by: Charlie Drage <[email protected]>

cdrage force-pushed the add-examples-page branch from ab53d85 to 2e21ee2 Compare October 23, 2024 19:16

benoitf approved these changes Oct 23, 2024

View reviewed changes

cdrage force-pushed the add-examples-page branch 2 times, most recently from bfedf6d to 2e21ee2 Compare October 23, 2024 19:39

cdrage enabled auto-merge (squash) October 23, 2024 19:39

cdrage merged commit 1223e41 into podman-desktop:main Oct 23, 2024
10 checks passed

		@@ -0,0 +1,56 @@
		<script lang="ts">
		import type { Example, Category } from '../../shared/src/models/examples';

	<a class="no-underline" href={href}>
	<a class="no-underline" {href}>

feat: add examples page #942

feat: add examples page #942

Conversation

cdrage commented Oct 17, 2024 • edited Loading

What does this PR do?

Screenshot / video of UI

What issues does this PR fix or reference?

How to test this PR?

cdrage commented Oct 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdrage Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deboer-tim left a comment

Choose a reason for hiding this comment

cdrage commented Oct 21, 2024 • edited Loading

cdrage commented Oct 21, 2024 • edited Loading

deboer-tim commented Oct 21, 2024

cdrage commented Oct 22, 2024

deboer-tim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdrage commented Oct 22, 2024

deboer-tim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdrage commented Oct 23, 2024 • edited Loading

benoitf commented Oct 23, 2024

benoitf commented Oct 23, 2024

cdrage commented Oct 23, 2024

cdrage commented Oct 17, 2024 •

edited

Loading

cdrage Oct 21, 2024 •

edited

Loading

cdrage commented Oct 21, 2024 •

edited

Loading

cdrage commented Oct 21, 2024 •

edited

Loading

cdrage commented Oct 23, 2024 •

edited

Loading