-
Notifications
You must be signed in to change notification settings - Fork 294
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
Implement ListItemButtonIcon component #6898
Comments
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
It can decide to be a dropdown with the
Moreover, it also manage confirmation dialogs when a UI building block component should not be managing such logic
based on those, I am not in favor of moving this component to the UI library. |
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 |
Thanks @benoitf for the detailed response :) !
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
My proposal When contextMy 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
We would reduce the complexity of ListItemButtonIcon.svelte. Confirmation dialogThe 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. |
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 #7233 will be merge I will take a deeper look at this particular component, but will put postponed on this task atm |
Solution proposal to free the components of the internal dependencies rejected #7658 (review) |
We cannot complete #376 because the issue to provide the component has stalled: containers/podman-desktop#6898 In the meantime we can do two things: - Pick up light mode support by copying the current code from Podman Desktop. - Remove the menu property, so that we can remove the unused dependency on the the DropdownMenuItem component. (if we need dropdown support in the future, we should use/require the component) Related to #376. Part of #536. Fixes #371. Signed-off-by: Tim deBoer <git@tdeboer.ca>
We cannot complete #376 because the issue to provide the component has stalled: containers/podman-desktop#6898 In the meantime we can do two things: - Pick up light mode support by copying the current code from Podman Desktop. - Remove the menu property, so that we can remove the unused dependency on the the DropdownMenuItem component. (if we need dropdown support in the future, we should use/require the component) Related to #376. Part of #536. Fixes #371. Signed-off-by: Tim deBoer <git@tdeboer.ca>
No description provided.
The text was updated successfully, but these errors were encountered: