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

Add loading indicator to form modals #1022

Merged
merged 5 commits into from
Dec 11, 2024
Merged

Conversation

elfjes
Copy link
Collaborator

@elfjes elfjes commented Dec 2, 2024

Adds a loading indicator to the _base_form_modal that can be toggled using hx-indicator and adds this hx-indicator to the bulk modals

Other modals may also implement thix by setting hx-indicator

from Uninett/argus-htmx-frontend#237

@codecov-commenter
Copy link

codecov-commenter commented Dec 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.19%. Comparing base (103abb6) to head (c485444).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1022   +/-   ##
=======================================
  Coverage   81.19%   81.19%           
=======================================
  Files         140      140           
  Lines        5074     5074           
=======================================
  Hits         4120     4120           
  Misses        954      954           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hmpf hmpf requested a review from podliashanyk December 2, 2024 08:55
@hmpf hmpf added the frontend Affects frontend label Dec 3, 2024
Copy link
Contributor

@podliashanyk podliashanyk left a comment

Choose a reason for hiding this comment

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

What this PR essentially does is adds loading indicator to submit action in any modal, not just bulk. This should either be made clear, or logic should be moved to src/argus/htmx/templates/htmx/incidents/_base_incident_update_modal.html instead.

I am personally not too thrilled about placement of the loading indicator over the text of the submit button. I would suggest putting the indicator on the whole modal, if inside _baso_form_modal:

<dialog id="{{ dialog_id }}" class="modal">
  <div class="modal-box card card-compact shadow-xl loading-box">
...

(the contents of .modal-box will need to be wrapped in a div for that to work).

Screenshot 2024-12-06 at 09 55 55

Or we could put the indicator below the input fields in each relevant modal.
Screenshot 2024-12-06 at 09 57 10

Submit button should also be disabled while request is loading, but this is a polish for another PR, unless we land on the option with keeping the indicator on the submit button. Button becomes grayed-out in that case and should definitely become disabled because of that.

src/argus/htmx/templates/htmx/_base_form_modal.html Outdated Show resolved Hide resolved
@elfjes elfjes changed the title Bulk action loading indicator Form modal loading indicator Dec 6, 2024
@elfjes elfjes marked this pull request as draft December 10, 2024 08:07
@elfjes elfjes force-pushed the bulk-action-indicator branch from fb93378 to c260552 Compare December 11, 2024 08:04
@elfjes elfjes marked this pull request as ready for review December 11, 2024 08:38
Copy link
Contributor

@podliashanyk podliashanyk left a comment

Choose a reason for hiding this comment

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

Great! 🙌

<button class="btn">{{ cancel_text }}</button>
</div>
<div class="modal-box card card-compact shadow-xl loading-box">
<div class="w-full">
Copy link
Contributor

Choose a reason for hiding this comment

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

w-full seems redundant

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought so too, but it wouldn't render correctly for me :(

Maybe it's something in our modal that we do differently, I can have a look

Copy link
Contributor

Choose a reason for hiding this comment

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

We can just keep it 😌

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ugh css is weird. Can't really figure it out, but something in our html requires that w-full but I don't know what. https://github.com/GEANT/geant-argus/blob/develop/src/geant_argus/geant_argus/templates/htmx/incidents/_incident_clear_modal.html

If it's not in your way, let's keep it in there :)

Copy link
Contributor

Choose a reason for hiding this comment

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

CSS is weird indeed 😄

@elfjes elfjes changed the title Form modal loading indicator Add loading indicator to form modals Dec 11, 2024
@hmpf hmpf merged commit 5edf7b9 into Uninett:master Dec 11, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend Affects frontend
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants