Implement ListItemButtonIcon component

[jeffmaury]

[axel7083]

I do not like this component, it feels like it is trying to do everything when it should not.

IMO a component should not decide if it wants to be a dropdown or not.

It is over complicated with some context logic, making it depends on the context interface and logic associated

https://github.com/containers/podman-desktop/blob/fff043eafff890906e22876a74661687ff1bdbe8/packages/renderer/src/lib/ui/ListItemButtonIcon.svelte#L18

It can decide to be a dropdown with the menu property

https://github.com/containers/podman-desktop/blob/fff043eafff890906e22876a74661687ff1bdbe8/packages/renderer/src/lib/ui/ListItemButtonIcon.svelte#L116

Moreover, it also manage confirmation dialogs when a UI building block component should not be managing such logic

https://github.com/containers/podman-desktop/blob/fff043eafff890906e22876a74661687ff1bdbe8/packages/renderer/src/lib/ui/ListItemButtonIcon.svelte#L91

based on those, I am not in favor of moving this component to the UI library.

cc @deboer-tim @benoitf

[benoitf]

IMO a component should not decide if it wants to be a dropdown or not.

you can think it as a responsive design, component change the way it's being displayed based on the width of the window.

If I remember, original idea was to keep the same component for actions without duplicating items, so it's either included in the details or in the list.

Also what would be your proposal as I guess it would implies some duplication or move to a code declaration vs UI component

[axel7083]

Thanks @benoitf for the detailed response :) !

IMO a component should not decide if it wants to be a dropdown or not.

you can think it as a responsive design, component change the way it's being displayed based on the width of the window.

I see, and have a better understand now, but responsive design is usually not based on component properties, but screen sizes so it feel a bit weird to me... but let's go with it

If I remember, original idea was to keep the same component for actions without duplicating items, so it's either included in the details or in the list.

Also what would be your proposal as I guess it would implies some duplication or move to a code declaration vs UI component

My proposal

When context

My first proposal would be to remove the context dependency from the component. If we really want to avoid repeating code I would create a component

<When context={} expression={}></when> which would be podman desktop specific.

We would reduce the complexity of ListItemButtonIcon.svelte.

Confirmation dialog

The second proposal would be to remove the confirmation dialog. IMO this is a backend responsibility, it can feel easy to have it on this component, but it add extra logic to it, and we cannot expose the confirmation dialog to the ui library as the window.showMessageBox would not be exposed in the webview. This is an internal ipc.

Those two proposition seems to be necessary to be able to expose such package.

[deboer-tim]

I agree this component does too much and should not be exposed as-is. The core use is just creating actions (icon, title, function) that can be displayed in individual buttons (e.g. actions in a table row), in a button bar (e.g. Resources), or in a drop-down. I think it's only the first that we need to expose now so could keep things simple (except for any required refactoring...) and defer the rest.

Given how we allow extensions to contribute actions to Podman Desktop I'm tempted to say this should all use the same command pattern, but that would make it too hard to add a simple icon that calls a function. Still, it would be nice if there was some alignment here so that you can create a button from either command id or icon/title/function. 🫤

[axel7083]

I agree this component does too much and should not be exposed as-is. The core use is just creating actions (icon, title, function) that can be displayed in individual buttons (e.g. actions in a table row), in a button bar (e.g. Resources), or in a drop-down. I think it's only the first that we need to expose now so could keep things simple (except for any required refactoring...) and defer the rest.

Given how we allow extensions to contribute actions to Podman Desktop I'm tempted to say this should all use the same command pattern, but that would make it too hard to add a simple icon that calls a function. Still, it would be nice if there was some alignment here so that you can create a button from either command id or icon/title/function. 🫤

After https://github.com/containers/podman-desktop/pull/7233 will be merge I will take a deeper look at this particular component, but will put postponed on this task atm

[axel7083]

Solution proposal to free the components of the internal dependencies rejected https://github.com/containers/podman-desktop/pull/7658#pullrequestreview-2138469566

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had activity in the last 6 months. It will be closed in 30 days if no further activity occurs. Please feel free to leave a comment if you believe the issue is still relevant. Thank you for your contributions!

[github-actions[bot]]

This issue has been automatically closed because it has not had any further activity in the last 30 days. Thank you for your contributions!