Skip to content

implement filter groups#11

Merged
d-flood merged 20 commits intomainfrom
DAR-1228-implement-filter-groups
Aug 12, 2025
Merged

implement filter groups#11
d-flood merged 20 commits intomainfrom
DAR-1228-implement-filter-groups

Conversation

@d-flood
Copy link
Collaborator

@d-flood d-flood commented Jul 24, 2025

Change Summary

This PR adds support for grouping filters in both collapsible and non-collapsible groups. It is not a breaking change since the filtersDefinition will be parsed for a flat definition if it is an object, but if it is an array then it will be parsed as an array of filter groupings. I included animations for the collapse and expand actions.

There are also a few customization updates such as making the search bar part of an overridable slot.

The logic here was tricky, so I have added a lot of tests (with lots of help from Sonnet 4, especially around the mock data).

Related issue number

Jira DAR-1228

PR Type

  • Feature
  • Bugfix
  • Refactor/optimization
  • Test Coverage
  • Documentation
  • Revert
  • Release

Engineer Checklist

  • The PR title is clear and descriptive.
  • Satisfied all pre-commit hooks locally, manually tested the changes and investigated potential regressions.
  • Have checked files to be committed to ensure no sensitive information or unwanted files are included.
  • Added or modified tests that cover the codebase changes made in this diff.
  • All automated checks and tests pass during CI.
  • Documentation and code comments have been added (where applicable).
  • Any hacks or questionable design decisions have been highlighted in the code and in the PR.
  • Does the PR require any infrastructure changes before deployment? Detail them below if so.
  • Are there any other outstanding blockers or pre-requisites that must be satisfied elsewhere in order for this work to be released?
  • If this is a feature PR, add this PR to the summary of the Dev -> Prod PR or open that PR if necessary. This helps us track the contents of production deploys.
  • Are there any other required post-deployment actions? If so, detail them below AND on a develop to main PR.

@d-flood d-flood requested a review from mutherr July 24, 2025 21:21
Copy link
Contributor

@mutherr mutherr left a comment

Choose a reason for hiding this comment

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

Overall this looks good, the tests look extremely thorough and the filters seem to behave well in my local testing.

However, I'm not sure the filters are being loaded from the url properly. If I try to navigate to http://localhost:5173/index1?page=1&Classification=Species&Crystal+system=Monoclinic (the page I arrive at after checking the first filter in the first group, it takes me to http://localhost:5173/index1?page=1&Classification=Species, the unfiltered species search page. If this is functionality we care about, maybe we should make sure it still works in the new filter system?

@d-flood
Copy link
Collaborator Author

d-flood commented Jul 29, 2025

Overall this looks good, the tests look extremely thorough and the filters seem to behave well in my local testing.

However, I'm not sure the filters are being loaded from the url properly. If I try to navigate to http://localhost:5173/index1?page=1&Classification=Species&Crystal+system=Monoclinic (the page I arrive at after checking the first filter in the first group, it takes me to http://localhost:5173/index1?page=1&Classification=Species, the unfiltered species search page. If this is functionality we care about, maybe we should make sure it still works in the new filter system?

Yeah, I noticed a little while ago that we had lost refresh stability. You can see the same behavior on Tsumeb now. We should fix it, but this PR did not introduce it.

@mutherr
Copy link
Contributor

mutherr commented Jul 29, 2025

Oh yeah I see what you mean. hmm

@mutherr mutherr self-requested a review July 30, 2025 15:08
Copy link
Contributor

@mutherr mutherr left a comment

Choose a reason for hiding this comment

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

Ok, now everything seems good. Thanks for the tests!

@d-flood d-flood merged commit f7255bc into main Aug 12, 2025
1 check passed
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.

2 participants