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 dropdowns to profiles page #1118

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Add dropdowns to profiles page #1118

wants to merge 5 commits into from

Conversation

hmpf
Copy link
Contributor

@hmpf hmpf commented Jan 10, 2025

Closes #1096

@hmpf hmpf requested a review from podliashanyk January 10, 2025 14:05
@hmpf hmpf self-assigned this Jan 10, 2025
@hmpf hmpf added the frontend Affects frontend label Jan 10, 2025
Copy link

github-actions bot commented Jan 10, 2025

Test results

   10 files  1 060 suites   38m 13s ⏱️
  536 tests   535 ✅  1 💤 0 ❌
5 360 runs  5 350 ✅ 10 💤 0 ❌

Results for commit 0177b93.

♻️ This comment has been updated with latest results.

@codecov-commenter
Copy link

codecov-commenter commented Jan 10, 2025

Codecov Report

Attention: Patch coverage is 43.82022% with 50 lines in your changes missing coverage. Please review.

Project coverage is 79.12%. Comparing base (bfa8920) to head (0177b93).

Files with missing lines Patch % Lines
src/argus/htmx/notificationprofile/views.py 38.15% 47 Missing ⚠️
src/argus/htmx/incident/filter.py 50.00% 2 Missing ⚠️
src/argus/htmx/widgets.py 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1118      +/-   ##
==========================================
- Coverage   79.67%   79.12%   -0.55%     
==========================================
  Files         141      141              
  Lines        5288     5371      +83     
==========================================
+ Hits         4213     4250      +37     
- Misses       1075     1121      +46     

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

@hmpf hmpf force-pushed the dropdowns-profiles-page branch 3 times, most recently from 9c7466b to 63dd879 Compare January 14, 2025 09:13
@hmpf hmpf marked this pull request as ready for review January 14, 2025 09:14
@johannaengland johannaengland linked an issue Jan 14, 2025 that may be closed by this pull request
Copy link
Contributor

@johannaengland johannaengland left a comment

Choose a reason for hiding this comment

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

This looks quite strange, when I try changing my selection of filters the dropdown and the box usually showing the selected filters disappear and I get the error

File "/home/johanna/Argus/.venv/lib/python3.12/site-packages/django/template/base.py", line 903, in _resolve_lookup
    raise VariableDoesNotExist(
django.template.base.VariableDoesNotExist: Failed lookup for key [class] in {'placeholder': 'select destination...', 'id': 'id_destinations'}

Screenshots:
Before:
Screenshot from 2025-01-15 08-41-47
After selecting:
Screenshot from 2025-01-15 08-41-52
After clicking outside of the dropdown
Screenshot from 2025-01-15 08-41-59

@hmpf
Copy link
Contributor Author

hmpf commented Jan 16, 2025

This looks quite strange, when I try changing my selection of filters the dropdown and the box usually showing the selected filters disappear /../

This should have been fixed.

@hmpf hmpf added the nonews No news fragment is necessary for this PR (e.g. refactoring, cleanups, workflow/development changes) label Jan 16, 2025
@hmpf hmpf force-pushed the dropdowns-profiles-page branch from fb1720e to 2429e13 Compare January 16, 2025 11:35
@hmpf
Copy link
Contributor Author

hmpf commented Jan 16, 2025

Lesson relearnt:

  • A ModelForm always has self.instance set. On create (before first save), pk is None (id does not always exist, pk always does)
  • A classy CreateView does not have self.object until after form_valid()
  • ModelChoiceFields (what you get with ModelForms for foreign keys and many2many fields) uses the queryset to validate the selection. If you change the queryset manually in the form __init__ as we do (eg. to limit it to user), choices must also be recomputed.

Copy link
Contributor

@johannaengland johannaengland left a comment

Choose a reason for hiding this comment

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

Why does this PR have a timeslot polish in it? That seems like it should be a separate PR

And I'm not sure if this is a polish or it is fine that is behaving like this, but if on an existing profile I (un)select filters/destinations and then not click on Save, but reload the page instead, then what the filter/destination field says is reset back to what the profile correctly has, but the checkboxes are still selected, which might trick the user into thinking it has been updated

Screenshots:
Before:
image

Selecting another filter:
image

Not clicking on Save, but reloading the page instead:
image

{{ form.as_div }}
{{ formset.as_div }}
<div class="card-actions justify-end">
{% if form.instance %}
Copy link
Contributor

Choose a reason for hiding this comment

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

For some reason if I click the Create button it still shows me Save and Delete

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@hmpf
Copy link
Contributor Author

hmpf commented Jan 17, 2025

Why does this PR have a timeslot polish in it? That seems like it should be a separate PR

So that's where those patches disappeared to! Thx.

@hmpf hmpf force-pushed the dropdowns-profiles-page branch from 102d0f5 to 9286009 Compare January 17, 2025 12:04
@hmpf
Copy link
Contributor Author

hmpf commented Jan 17, 2025

I have removed the timeslots changes.

@hmpf
Copy link
Contributor Author

hmpf commented Jan 17, 2025

And I'm not sure if this is a polish or it is fine that is behaving like this, but if on an existing profile I (un)select filters/destinations and then not click on Save, but reload the page instead, then what the filter/destination field says is reset back to what the profile correctly has, but the checkboxes are still selected, which might trick the user into thinking it has been updated

I have seen that myself but don't know how to fix it.

@hmpf hmpf requested a review from johannaengland January 17, 2025 12:59
@johannaengland
Copy link
Contributor

And I'm not sure if this is a polish or it is fine that is behaving like this, but if on an existing profile I (un)select filters/destinations and then not click on Save, but reload the page instead, then what the filter/destination field says is reset back to what the profile correctly has, but the checkboxes are still selected, which might trick the user into thinking it has been updated

I have seen that myself but don't know how to fix it.

We can make it a polish issue and see, if we figure it out at some point

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 about complaints from sonarqube? Also styles.css needs to be updated.

Works otherwise. We need to start looking at #1138 soon after this one is merged, otherwise the UX on error is quite poor

{% endblock field_control %}>
<div tabindex="0"
role="button"
class="show-selected-box input input-accent input-bordered input-md border overflow-y-auto min-h-8 h-auto max-h-16 max-w-xs leading-tight flex flex-wrap items-center gap-0.5">
<p class="text-base-content/50">{{ widget.attrs.placeholder }}</p>
{% if not widget.has_selected %}<p class="text-base-content/50">{{ widget.attrs.placeholder }}</p>{% endif %}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is out of scope for this PR. If we keep it here, we should close #1100.
I also don't like how "jumpy" the width of the dropdown multiselect field becomes, we need a separate issue to fix it. We need both a minimum width and some transitions to smooth out the changes in width.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The height is also fixed in the current dropdown: specialized for the filter box. It looks better with a higher min-height elsewhere.

@hmpf
Copy link
Contributor Author

hmpf commented Jan 20, 2025

/../ if on an existing profile I (un)select filters/destinations and then not click on Save, but reload the page instead, then what the filter/destination field says is reset back to what the profile correctly has, but the checkboxes are still selected, /../

I have seen that myself but don't know how to fix it.

We can make it a polish issue and see, if we figure it out at some point

Does the "autocomplete" commit fix this for you? Also, it is apparently a firefox only feature/problem.

@podliashanyk
Copy link
Contributor

podliashanyk commented Jan 20, 2025

There is another thing I noticed: both "Filters" and "Destinations" multiselects on profiles page are wrapped with fieldset tag. I don't see any place where it comes from explicitly. Is it some class based views thing? I would like to drop wrapping those fields with fieldset, since they are not actually fieldsets so it becomes confusing. But it is a nice-to-have.

@hmpf
Copy link
Contributor Author

hmpf commented Jan 20, 2025

There is another thing I noticed: both "Filters" and "Destinations" multiselects on profiles page are wrapped with fieldset tag. I don't see any place where it comes from explicitly. Is it some class based views thing? I would like to drop wrapping those fields with fieldset, since they are not actually fieldsets so it becomes confusing. But it is a nice-to-have.

It's in the default diango template for as_div and with the merge of #1142 it can be altered by changing the file src/argus/htmx/templates/django/forms/div.html. It has already been altered a little, I've moved the "errors"-block for a specific field below that field. Compare with the django 4.2 source code: django/forms/templates/django/forms/div.html.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend Affects frontend nonews No news fragment is necessary for this PR (e.g. refactoring, cleanups, workflow/development changes)
Projects
Status: Changes requested
4 participants