hypothesis / lms

LTI app for integrating with learning management systems
BSD 2-Clause "Simplified" License
46 stars 14 forks source link

Create hook to track selected dashboard filters in query string #6466

Closed acelaya closed 2 months ago

acelaya commented 2 months ago

This PR adds a new useDashboardQueryFilters hook that can be used together with DashboardActivityFilters to a) initialize selected filters based on query parameters on active page, and b) persist selected filters in the query string, so that they can be propagated or bookmarked.

One change needed to achieve this was that DashboardActivityFilters now only handles the IDs of selected elements, instead of the full objects.

https://github.com/user-attachments/assets/d0081269-ec1f-4d75-9172-e03da930ea49

[!NOTE]
As of now, every filter change will replace the last history entry. We can change this later with { replace: false }, which makes it internally call history.pushState() instead of history.replaceState().

Testing steps

  1. Open the dashboard.
  2. Navigate to "All courses".
  3. Change filters and observe how they reflected in the query string.
  4. Reload the page. Filters from the query string should be applied automatically.

TODO

robertknight commented 2 months ago

This does indeed create a bunch of identical looking history entries as you navigate around, which is unfortunate.

History entries

This issue is not exclusive to Hypothesis though. I think I am leaning towards replacing rather than pushing a URL here. This would mean that when you navigate around the dashboard via links to "major" new pages, that would push a URL so you could go back and forwards between them, but within each view we'd only remember the last-used state. The downside of replacing the URL is that you then couldn't use back/forwards to switch between filter configurations.

acelaya commented 2 months ago

Something that I think may be surprising for other developers in future is that DashboardActivityFilters takes selectedCourses etc. as props like an uncontrolled component, but is also responsible for initializing this state from the environment.

Yeah, this is a good point.

I think a better approach would be to create a hook which wraps the logic and returns the parsed query and the callback to update it, and let components using DashboardActivityFilters decide if they want to use it.

That'll keep DashboardActivityFilters truly controlled, and decoupled from the query handling logic, but let consumers plug-in the functionality in a straightforward way.

I'll move this to draft while I make the adjustments.

acelaya commented 2 months ago

~One apparently unrelated test broke. I'm checking why.~

EDIT: Fixed.

robertknight commented 2 months ago

If a query string parameter specifies an ID that is invalid, the current experience is that the corresponding dropdown selection is blank and the filter will be empty:

Invalid ID

Perhaps we should do something here to make it more obvious why the results are blank, such as rendering Student not found as the dropdown value in this case. The user can reset the view to a non-empty state by changing the filters.

Edit: The suggested will be more difficult when we start to make use of pagination for the responses, since items may be missing from the dropdown values if they are valid but don't appear in the loaded pages.

robertknight commented 2 months ago

If the query string has a single invalid ID in it, the dropdown is blank. If you then click the dropdown you see nothing selected. If you make a selection, it shows one item in the list as being selected but the content of the select reports there are two items selected:

Invalid ID selection count mismatch

A way to avoid this would be to remove any invalid IDs from the selection when it is changed.

acelaya commented 2 months ago

If a query string parameter specifies an ID that is invalid, the current experience is that the corresponding dropdown selection is blank and the filter will be empty:

If the query string has a single invalid ID in it, the dropdown is blank. If you then click the dropdown you see nothing selected. If you make a selection, it shows one item in the list as being selected but the content of the select reports there are two items selected:

I would like to focus on the happy path for now, as determining if the query params are valid or not depends on the loading of the values for the dropdown.

With pagination involved, there could be cases in which there are actually valid values in the query, but those are not in the first page, so it's not possible to tell right away.

I will do a bit of experimentation and consider possible options once I focus on that.