Sorting header for members list closes #152 #175 #166

Merged
moritz merged 11 commits from feature/152_sorting_default_fields into main 2025-10-30 16:44:50 +01:00
Owner

Description of the implemented changes

The changes were:

  • Bugfixing
  • New Feature
  • Breaking Change
  • Refactoring

What has been changed?

Definition of Done

Code Quality

  • No new technical depths
  • Linting passed
  • Documentation is added were needed

Accessibility

  • New elements are properly defined with html-tags
  • Colour contrast follows WCAG criteria
  • Aria labels are added when needed
  • Everything is accessible by keyboard
  • Tab-Order is comprehensible
  • All interactive elements have a visible focus

Testing

  • Tests for new code are written
  • All tests pass
  • axe-core dev tools show no critical or major issues

Additional Notes

## Description of the implemented changes The changes were: - [ ] Bugfixing - [X] New Feature - [ ] Breaking Change - [ ] Refactoring <!--- Describe the goal of the PR in a few words --> ## What has been changed? <!--- List the things you changed --> ## Definition of Done ### Code Quality - [x] No new technical depths - [ ] Linting passed - [x] Documentation is added were needed ### Accessibility - [x] New elements are properly defined with html-tags - [x] Colour contrast follows WCAG criteria - [ ] Aria labels are added when needed - [x] Everything is accessible by keyboard - [x] Tab-Order is comprehensible - [x] All interactive elements have a visible focus ### Testing - [x] Tests for new code are written - [x] All tests pass - [ ] axe-core dev tools show no critical or major issues ## Additional Notes <!--- Add any additional information for the reviewers here -->
carla added 1 commit 2025-09-26 11:11:40 +02:00
carla added 3 commits 2025-09-30 16:23:44 +02:00
carla force-pushed feature/152_sorting_default_fields from 9b64e35cce to 3b3ef796a4 2025-09-30 16:28:47 +02:00 Compare
Author
Owner

I implemented the Sort function as LiveComponent which we could reuse.

I added an issue for the aria-sort label which I then left out: #174

I kept the TableComponents file because this sort button is still used in users. As you are currently working on users - members, I thought it would be better to take care of userslist afterwards?

I implemented the Sort function as LiveComponent which we could reuse. I added an issue for the aria-sort label which I then left out: https://git.local-it.org/local-it/mitgliederverwaltung/issues/174 I kept the TableComponents file because this sort button is still used in users. As you are currently working on users - members, I thought it would be better to take care of userslist afterwards?
carla added 1 commit 2025-09-30 16:46:12 +02:00
formatting
Some checks failed
continuous-integration/drone/push Build is failing
5a17a9e929
carla changed title from WIP: Sorting header for members list closes#152 to Sorting header for members list closes#152 2025-09-30 16:46:28 +02:00
requested reviews from moritz, simon 2025-09-30 16:46:41 +02:00
moritz force-pushed feature/152_sorting_default_fields from 5a17a9e929 to 78cbd3cf80 2025-10-09 16:33:04 +02:00 Compare
Owner

If the rows are sorted by a specific column the column header shows a single arrow in the sorting direction. The other rows show a double arrow (up and down). If I sort for another column the double arrow switches into a single arrow, but the previous column header arrow keeps the same. So it's not possible to see which column is the column sorted by. It would be good to reset the sorting arrows of the previous columns. Maybe something more obvious to highlight which is the current sort column would be nice too.

If the rows are sorted by a specific column the column header shows a single arrow in the sorting direction. The other rows show a double arrow (up and down). If I sort for another column the double arrow switches into a single arrow, but the previous column header arrow keeps the same. So it's not possible to see which column is the column sorted by. It would be good to reset the sorting arrows of the previous columns. Maybe something more obvious to highlight which is the current sort column would be nice too.
moritz force-pushed feature/152_sorting_default_fields from 78cbd3cf80 to e8d440f74f 2025-10-09 17:56:01 +02:00 Compare
moritz requested changes 2025-10-09 18:03:56 +02:00
Dismissed
moritz left a comment
Owner

Looks very good 🚀
I added many small things. Main issues to fix are the search + sort bug and the resetting of previous sort arrows.

Looks very good 🚀 I added many small things. Main issues to fix are the search + sort bug and the resetting of previous sort arrows.
@ -0,0 +2,4 @@
@moduledoc """
Sort Header that can be used as column header and sorts a table:
Props:
- field: atom() # AshField for sorting
Owner

ambiguous Unicode character - in comment.

ambiguous Unicode character `-` in comment.
carla marked this conversation as resolved
@ -0,0 +3,4 @@
Sort Header that can be used as column header and sorts a table:
Props:
- field: atom() # AshField for sorting
- label: string() # Column Heading (can be aan heex templyte)
Owner

typo templyte

typo `templyte`
carla marked this conversation as resolved
@ -0,0 +48,4 @@
end
# -------------------------------------------------
# Hilfsfunktionen für ARIAAttribute & IconSVG
Owner

ambiguous Unicode character - in comment.

ambiguous Unicode character - in comment.
carla marked this conversation as resolved
@ -40,2 +16,2 @@
|> assign(:query, q)
|> assign(:members, members)}
# We call handle params to use the query from the URL
{:noreply, socket} = handle_params(params, nil, socket)
Owner

https://hexdocs.pm/phoenix_live_view/Phoenix.LiveView.html
"The life-cycle is: mount/3 -> handle_params/3 -> render/1"
Therefore handle_params is called twice, in your mount/3 and afterwards by the LiveView. Each handdle_params loads all the members. Is this line necessary at all? If I just remove it it still works for me.

https://hexdocs.pm/phoenix_live_view/Phoenix.LiveView.html "The life-cycle is: mount/3 -> handle_params/3 -> render/1" Therefore handle_params is called twice, in your mount/3 and afterwards by the LiveView. Each handdle_params loads all the members. Is this line necessary at all? If I just remove it it still works for me.
moritz marked this conversation as resolved
@ -111,0 +93,4 @@
}
# "/members" is the path you defined in router.ex
new_path = "/members?" <> URI.encode_query(query_params)
Owner

Best practice: instead of hardcoded paths, render path dynamically:

current_path = socket.view.__live__() |> elem(0)
new_path = current_path <> "?" <> URI.encode_query(query_params)

Also the ~p sigil could be used for creating a path.

Best practice: instead of hardcoded paths, render path dynamically: ``` current_path = socket.view.__live__() |> elem(0) new_path = current_path <> "?" <> URI.encode_query(query_params) ``` Also the `~p` sigil could be used for creating a path.
Author
Owner

I tried that already before and did not find a simple solution for getting the current path. So I would keep that static for now.

I tried that already before and did not find a simple solution for getting the current path. So I would keep that static for now.
moritz marked this conversation as resolved
@ -111,0 +106,4 @@
# Function to handle search
@impl true
def handle_info({:search_changed, q}, socket) do
Owner

This function ignores the sorting. This leads to reset the sorting while searching.
To avoid duplicate code I would recommend to integrate the search logic into load_members.

This function ignores the sorting. This leads to reset the sorting while searching. To avoid duplicate code I would recommend to integrate the search logic into `load_members`.
moritz marked this conversation as resolved
@ -0,0 +9,4 @@
assert view |> element("[data-testid='first_name']")
end
end
Owner

Maybe some more tests like:

  • Test that all sortable headers exist
  • Test initial state shows correct icon
  • Test aria-label
Maybe some more tests like: - Test that all sortable headers exist - Test initial state shows correct icon - Test aria-label
moritz marked this conversation as resolved
@ -78,0 +79,4 @@
conn = conn_with_oidc_user(conn)
{:ok, view, _html} = live(conn, "/members")
# The component data test ids are built as "<field>"
Owner

ambiguous Unicode character - in comment.

ambiguous Unicode character `-` in comment.
moritz marked this conversation as resolved
@ -78,0 +87,4 @@
# The LiveView pushes a patch with the new query params
assert_patch(view, "/members?sort_field=email&sort_order=asc")
Owner

ambiguous Unicode character - in comment.

ambiguous Unicode character `-` in comment.
moritz marked this conversation as resolved
moritz reviewed 2025-10-10 12:53:50 +02:00
@ -112,0 +81,4 @@
active_id = :"sort_#{new_field}"
# Update the SortHeader to
send_update(MvWeb.Components.SortHeaderComponent,
Owner

@moritz wrote in #166 (comment):

If the rows are sorted by a specific column the column header shows a single arrow in the sorting direction. The other rows show a double arrow (up and down). If I sort for another column the double arrow switches into a single arrow, but the previous column header arrow keeps the same. So it's not possible to see which column is the column sorted by. It would be good to reset the sorting arrows of the previous columns. Maybe something more obvious to highlight which is the current sort column would be nice too.

maybe something like this could work to reset the old header component:

   old_id = :"sort_#{old_field}"

    # Update the SortHeader to
    send_update(MvWeb.Components.SortHeaderComponent,
      id: old_id,
      sort_field: new_field,
      sort_order: new_order
    )
@moritz wrote in https://git.local-it.org/local-it/mitgliederverwaltung/pulls/166#issuecomment-13801: > If the rows are sorted by a specific column the column header shows a single arrow in the sorting direction. The other rows show a double arrow (up and down). If I sort for another column the double arrow switches into a single arrow, but the previous column header arrow keeps the same. So it's not possible to see which column is the column sorted by. It would be good to reset the sorting arrows of the previous columns. Maybe something more obvious to highlight which is the current sort column would be nice too. maybe something like this could work to reset the old header component: ``` old_id = :"sort_#{old_field}" # Update the SortHeader to send_update(MvWeb.Components.SortHeaderComponent, id: old_id, sort_field: new_field, sort_order: new_order ) ```
moritz marked this conversation as resolved
carla added 6 commits 2025-10-24 11:25:46 +02:00
carla changed title from Sorting header for members list closes#152 to Sorting header for members list closes#152 #175 2025-10-24 11:37:31 +02:00
carla changed title from Sorting header for members list closes#152 #175 to Sorting header for members list closes #152 #175 2025-10-24 11:37:40 +02:00
carla force-pushed feature/152_sorting_default_fields from 4633a4610b to 660d587cc1 2025-10-30 13:05:09 +01:00 Compare
moritz force-pushed feature/152_sorting_default_fields from 660d587cc1 to 8104451d35 2025-10-30 16:42:23 +01:00 Compare
moritz approved these changes 2025-10-30 16:44:36 +01:00
moritz left a comment
Owner

Looks very good 💯

Looks very good 💯
moritz merged commit c1f9750972 into main 2025-10-30 16:44:50 +01:00
moritz deleted branch feature/152_sorting_default_fields 2025-10-30 16:44:51 +01:00
Sign in to join this conversation.
No description provided.