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: using podman-desktop modals instead of custom one #210

Merged

Conversation

axel7083
Copy link
Contributor

@axel7083 axel7083 commented Feb 1, 2024

What does this PR do?

Uses the Podman-Desktop provided modals instead of custom one.

Screenshot / video of UI

podman-desktop-delete-modals.mp4

What issues does this PR fix or reference?

Fixes #203

How to test this PR?

@axel7083 axel7083 requested a review from a team as a code owner February 1, 2024 13:08
@axel7083 axel7083 requested review from benoitf, jeffmaury and feloy and removed request for jeffmaury February 1, 2024 13:08
@axel7083
Copy link
Contributor Author

axel7083 commented Feb 1, 2024

Not related directly to this PR, but the following podman-desktop/podman-desktop#5744 would be a nice touch !

Signed-off-by: axel7083 <[email protected]>
Signed-off-by: axel7083 <[email protected]>
Signed-off-by: axel7083 <[email protected]>
@axel7083 axel7083 added the kind/enhancement ✨ New feature or request label Feb 1, 2024
@axel7083 axel7083 requested a review from lstocchi February 5, 2024 10:06
@axel7083 axel7083 self-assigned this Feb 5, 2024
Copy link
Contributor

@lstocchi lstocchi left a comment

Choose a reason for hiding this comment

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

It seems that if there is some error while deleting we do not show anything to the user.
Maybe this can be done in a follow-up PR as you're just replacing the modal but, thinking at this future change, wouldn't it be better to await the showWarningMessage and have the deleteLocalModel outside the then?

@axel7083
Copy link
Contributor Author

axel7083 commented Feb 5, 2024

It seems that if there is some error while deleting we do not show anything to the user. Maybe this can be done in a follow-up PR as you're just replacing the modal but, thinking at this future change, wouldn't it be better to await the showWarningMessage and have the deleteLocalModel outside the then?

@lstocchi Yes, we could also use ShowError api inside the backend and try catch the deletion. But this is not in the scope of this PR, as the current behavior is not supporting it either

@lstocchi
Copy link
Contributor

lstocchi commented Feb 5, 2024

Yes, we could also use ShowError api inside the backend and try catch the deletion. But this is not in the scope of this PR, as the current behavior is not supporting it either.

So my first thought was wrong, by looking at the code we have a showErrorMessage if the delete action fails
https://github.com/projectatomic/ai-studio/blob/24802ec11ae0b6c3c4038f53b7ee6426693c6b7c/packages/backend/src/managers/modelsManager.ts#L156

Copy link
Contributor

@lstocchi lstocchi left a comment

Choose a reason for hiding this comment

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

If you could change the function "style" as commented by Jeff/Florent i'm good with it

Signed-off-by: axel7083 <[email protected]>
Signed-off-by: axel7083 <[email protected]>
@axel7083 axel7083 requested a review from lstocchi February 6, 2024 09:14
Copy link
Contributor

@lstocchi lstocchi left a comment

Choose a reason for hiding this comment

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

LGTM

@axel7083 axel7083 merged commit 786d748 into containers:main Feb 6, 2024
3 checks passed
mhdawson pushed a commit to mhdawson/podman-desktop-extension-ai-lab that referenced this pull request Nov 22, 2024
Send output from bootc-image-builder to build subdir
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement ✨ New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Delete model modal should use podman-desktop api for confirmation
4 participants