Support toggling different filter states independently #6056
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
robertknight
commented
Jan 5, 2024
src/sidebar/store/modules/filters.ts
Outdated
| */ | ||
| export type FilterKey = 'cfi' | 'page' | 'user'; | ||
| export type FilterKey = 'cfi' | 'pages' | 'user'; |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
robertknight
force-pushed
the
multiple-filter-active-states
branch
2 times, most recently
from
22140e4 to
3414a52
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6056 +/- ##
=======================================
Coverage 99.44% 99.45%
=======================================
Files 265 265
Lines 10149 10195 +46
Branches 2459 2474 +15
=======================================
+ Hits 10093 10139 +46
Misses 56 56 ☔ View full report in Codecov by Sentry. |
|
I need to check that the Edit: Reverted this change and updated code comments to save the next guy from making the same mistake. |
robertknight
force-pushed
the
multiple-filter-active-states
branch
from
3414a52 to
6b749dc
Compare
Store the focus filter activation state as a set of active filters (eg.
`{"user", "page"}`) instead of a boolean. This will allow the filters to be
toggled independently in future.
To support the existing UI where there is only a single toggle control,
`toggleFocusMode` retains the ability to toggle all configured filters. Once
the new search UI is rolled out for everyone, we can probably remove this.
- Try to better explain the context in which each of the filters is used - Re-organize the state fields so that user-applied filters are at the top, followed by the focus filter state
This unfortunately doesn't catch mistakes where incorrect property names are used because TS allows excess properties, but it does at least document what the expected keys are.
robertknight
force-pushed
the
multiple-filter-active-states
branch
from
6b749dc to
64624f1
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In order to support a UI like the one shown in #6006 (comment) it needs to be possible to toggle different filter modes independently. For example, if a teacher is grading an assignment with a page range specified, there will be filters for both the student being graded and the page range, and it should be possible to toggle them independently. This PR refactors the
focusActivestate in thefiltersstore module to support this.focusActivestate from a boolean to a set of enabled states and extends thetoggleFocusModestore action APIThere should be no functional changes , as the new capabilities are not used by the UI yet.