Listing pages filtering - #451
Conversation
Reusable backend core for validating listing query params and building the shared filter UI context, used by the blog, work and events listings. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Filter dropdowns, active-filter pills and the clear-all control shared by the listing pages, plus the JS component and styles that drive them. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The slug is used as the value in event-filter URLs. Includes a factory for test data. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Filters match both WorkPage and HistoricalWorkPage subtypes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Improve high contrast mode accessibility for submit button - Remove excess comments - Standardise use of SCSS styles for listing filters component
helenb
left a comment
There was a problem hiding this comment.
Thanks @jhancock532 this is looking really good. Most of my comments are pretty minor. Note I think my comments may come in two reviews as I had another tab open to look a the CSS and markup changes in tandem.
| @@ -0,0 +1,6 @@ | |||
| <div class="listing-header grid__listing-header mb-spacerSmallPlus lg:mb-spacerMedium"> | |||
| <h1 class="heading heading--mega">{{ item.title }}</h1> | |||
There was a problem hiding this comment.
I wonder - do we need to include the heading 1 and strapline in a special component here? The title-filters.html component needs to include the heading 1 because of the layout, but as the filters are laid out below the heading then we could just render the heading and strapline in the page template, and include the listing filters below. If we do need to keep it, can we add a yaml file for it.
There was a problem hiding this comment.
I added this in just to reduce a bit of code duplication, so we don't necessarily need this - I've reverted the change here - 4805d19
There was a problem hiding this comment.
Sorry for another nitpick but we now have the listing-header class outside of the listing-header component. If we do need a special class for the h1 here it should maybe return to the separate component after all.
|
Q: does this supersede #438 ? |
…of generic no results text
- Make focus styles for buttons consistent. - Use data attribute instead of matching on details element - Add comment reminder to update Jest markup - Revert change moving minor duplicate code to shared component
helenb
left a comment
There was a problem hiding this comment.
Thanks for all the fixes - this is looking great now - approving from a front-end perspective. It would be good if you can fix the one small remaining issue but it is non-blocking.
| sector_choices=[(s.slug, s.name) for s in related_sectors], | ||
| service_choices=[(s.slug, s.name) for s in related_services], | ||
| ) | ||
| form.is_valid() |
There was a problem hiding this comment.
Issue: I don't think you can access cleaned_data below if this is False. If the form is invalid, we should not apply filters. I can see the form is dropping incorrect values. Reading the calling code at this abstraction level I'd rather make it exhaustive based on values that is_valid may return.
| extra_url_params = {} | ||
| # Only offer Sectors and Services that are actually used on this listing. | ||
| related_sectors = Sector.objects.filter( | ||
| Q(pk__in=models.Subquery(self.works.values("workpage__related_sectors"))) |
There was a problem hiding this comment.
Question: Just a quick double check, does the related models here return PKs of the sectors or PKs of the through model instances?
Should the code explicitly use values_list with the PK field only to make the query behaviour easier to understand just by reading it?
Does values() do some form of implicit behaviour here to join those tables up?
Description of Changes Made
Adds sector / service filtering to the news and work listing pages, as well as timing / event type for the event listing page.
tbx/core/listing/)slugfield added on theEventTypesnippet, used in event-filter URLsHow to Test
Test locally by confirming news, work and event listing pages filter correctly. By default, the event listing page should only show upcoming events.
Confirm that the user can select multiple filter options from the drop-down checkboxes, and apply their filter choices with the filter button.
Confirm that when the page loads with filters applied, these filters are shown in the active filters session. Dismissing one of the active filters causes the page to reload with that filter no longer applied.
Confirm that with active filters applied, pressing the "Clear all filters" button then removes all the active filters from the page.
Check out the listing filters component in the pattern library: http://localhost:8000/pattern-library/pattern/patterns/molecules/listing-filters/listing-filters.html
Screenshots
Expand to see more
Event page, dark mode with event type filters.
New page, light mode with multiple filters
Work page, light mode with multiple filters, mobile
Pattern library component
MR Checklist
Unit tests
Documentation
Browser testing
Data protection
Light and dark mode
Accessibility
Sustainability
Pattern library