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

fix: button must have a spinner when progress true #189

Merged
merged 2 commits into from
Jan 31, 2024

Conversation

axel7083
Copy link
Contributor

Description

Fixe Button svelte component to have proper spinner when progress true

Screenshots

podman-spinner-progress.mp4

Related issues

Fixes #188

Tests

  • Regression tests has been added

@axel7083 axel7083 self-assigned this Jan 30, 2024
@axel7083 axel7083 requested a review from lstocchi January 30, 2024 11:43
@axel7083
Copy link
Contributor Author

cc @slemeur

@benoitf
Copy link
Collaborator

benoitf commented Jan 30, 2024

I think that when modifying code duplicated from the core of podman desktop you should first discuss in the upstream before applying what I call 'downstream/ai-studio' library because your manual change will be easily dropped/forgotten when for example ai-studio' would use the podman desktop button component

@benoitf
Copy link
Collaborator

benoitf commented Jan 30, 2024

here for example you could use an icon for the 'send request' button as well
When in progress, icon will be replaced by the spinner

@axel7083
Copy link
Contributor Author

axel7083 commented Jan 30, 2024

I think that when modifying code duplicated from the core of podman desktop you should first discuss in the upstream before applying what I call 'downstream/ai-studio' library because your manual change will be easily dropped/forgotten when for example ai-studio' would use the podman desktop button component

I am a bit reluctant to be force to keep some kind of upstream/downstream for the UI, when we usually already modify them a lot when we copy/past the components, as we are trying to limit the duplicate code. I hope, as well that in the future, the podman-desktop library will be more "standard" and such property as "inProgress" will be change, to match for example other component using "loading". They are a lot of inconsistency already, and the day we will have to migrate to the "podman-desktop" shared library will probably not be as easy as just replace the components, because most of them will be probably changed and adapted to be generics.

here for example you could use an icon for the 'send request' button as well When in progress, icon will be replaced by the spinner

But why would be want to spinner icon to be displayed if we provide the "inProgress" property to true...

Finally, I would prefer we have a clear discussion on the matter, and clearly state, what we want to do, if we want to keep the same components and property (and do not change the implementation) or we can customize them, this is an issue not related to this specific PR and should be addressed as we are already often changing them.

cc @jeffmaury

Copy link
Contributor

@jeffmaury jeffmaury 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 4d2ffb0 into main Jan 31, 2024
3 checks passed
@axel7083 axel7083 deleted the fix/188-progress-button branch January 31, 2024 13:35
mhdawson pushed a commit to mhdawson/podman-desktop-extension-ai-lab that referenced this pull request Nov 22, 2024
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.

submit button in playground should have a spinner when progress is true
3 participants