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

[#1123] Feature/search functionality in plan list view #484

Merged
merged 3 commits into from
Feb 23, 2023

Conversation

vaszig
Copy link
Contributor

@vaszig vaszig commented Feb 21, 2023

FYI, we use auto-submit forms (via js) in some components. We have this for input and select html tags but afaik we only use it for select now, that's why I removed input selector. Another solution would be to add some time delay in order to be easier for the user to type something in the search input (before the form is auto-submitted).

@codecov-commenter
Copy link

codecov-commenter commented Feb 21, 2023

Codecov Report

Merging #484 (980ffff) into develop (0ca5a95) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##           develop     #484    +/-   ##
=========================================
  Coverage    96.49%   96.50%            
=========================================
  Files          507      514     +7     
  Lines        18451    18561   +110     
=========================================
+ Hits         17805    17913   +108     
- Misses         646      648     +2     
Impacted Files Coverage Δ
src/open_inwoner/plans/forms.py 100.00% <100.00%> (ø)
src/open_inwoner/plans/tests/test_views.py 100.00% <100.00%> (ø)
src/open_inwoner/plans/views.py 97.87% <100.00%> (+0.03%) ⬆️
src/open_inwoner/urls.py 81.48% <0.00%> (-6.02%) ⬇️
src/open_inwoner/accounts/admin.py 93.58% <0.00%> (ø)
src/open_inwoner/accounts/views/cases.py 97.52% <0.00%> (ø)
src/open_inwoner/accounts/tests/factories.py 100.00% <0.00%> (ø)
.../open_inwoner/accounts/tests/test_profile_views.py 100.00% <0.00%> (ø)
..._inwoner/pdc/migrations/0049_auto_20230222_0901.py 100.00% <0.00%> (ø)
..._inwoner/pdc/migrations/0045_auto_20230221_0954.py 100.00% <0.00%> (ø)
... and 7 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@Bartvaderkin Bartvaderkin left a comment

Choose a reason for hiding this comment

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

Works pretty good. One extra feedback on existing code: I'd expect the bold title in the 'Samenwerking' column in the list also clickable, and to have something in the aria-label of the arrow for accessibility.

Also I noticed the filter widgets don't have a regular label so I'd expect they need something as well.

src/open_inwoner/plans/views.py Outdated Show resolved Hide resolved
src/open_inwoner/plans/views.py Outdated Show resolved Hide resolved
@vaszig
Copy link
Contributor Author

vaszig commented Feb 22, 2023

@Bartvaderkin I tried to fix everything you mentioned except for the labels in filters. We have the same approach in actions filters (and maybe somewhere else too), so if we do need them I will create a separate task for all of them.

@@ -51,7 +51,7 @@ <h1 class="h1">
<div class="table__item--notification-danger">{% trans "Actie vereist" %}</div>
{% endif %}
</td>
<td class="table__item">{% button text="" href=plan.get_absolute_url icon="arrow_forward" icon_outlined=True transparent=True %}</td>
<td class="table__item">{% button text="plan_detail" hide_text=True href=plan.get_absolute_url icon="arrow_forward" icon_outlined=True transparent=True %}</td>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be text=plan_detail (without quotes)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For me it doesn't work without quotes, because it then means it's a variable but I need a string here.
When I have the quotes I see the following:

image

Copy link
Contributor

@Bartvaderkin Bartvaderkin Feb 22, 2023

Choose a reason for hiding this comment

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

My example was bad, I meant text=plan.title, or something else useful that makes sense to an accessibility user.

Having literally aria-label="plan_detail" doesn't seem helpful.

@Bartvaderkin
Copy link
Contributor

It is possible the action filters weren't there when this was initially reviewed for accessibility.

Anyway, we need to have something for accessibility so if it is not part of this then a new ticket would be good.

@vaszig
Copy link
Contributor Author

vaszig commented Feb 22, 2023

Anyway, we need to have something for accessibility so if it is not part of this then a new ticket would be good.

Yes, you are right. Task #1183 https://taiga.maykinmedia.nl/project/open-inwoner/task/1183 wad created.

@vaszig vaszig requested a review from Bartvaderkin February 23, 2023 09:42
@alextreme alextreme merged commit 306e9fe into develop Feb 23, 2023
@alextreme alextreme deleted the feature/1123-plan-list-view-search branch February 23, 2023 09:48
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.

4 participants