-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Keep filters option when switching repos #226
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested - The filters are shown but they are not applied after switching the repo
Also, the search bar and the label filters are not persisted.
/** This copy of the filter is constantly updated when a change in filter is detected */ | ||
let currentDropdownFilter: DropdownFilter = { ...DEFAULT_DROPDOWN_FILTER }; | ||
|
||
/** This is the initial dropdown filter applied when first viewing a new repo */ | ||
let initialDropdownFilter: DropdownFilter = { ...DEFAULT_DROPDOWN_FILTER }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to store both initial and current Dropdown filters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both are unnecessary in hindsight. Initial dropdown filter variable has been removed.
Committed new changes that fixes functionality for status, type and sort filters. |
Actually, for CS2103 projects which is the main use case for WATcher, they will most likely be same or similar. It's up to you if you want to continue working on it or create a separate issue for someone else to pick up.
Yes, drop label selections that don't exist |
I have refactored the code into its own file called FiltersStore.ts and am using a static class to store the filters as they are edited, initialize new repos with previous filter values and clearing the store if filters are not kept. |
In hindsight, I think a better approach for consistency would be to have my FiltersStore.ts be the central home of the filters such that all components update and retrieve from FiltersStore. This would negate me having to copy filter changes and will be more developer friendly to understand. What do you think? |
dropdownFilter: DropdownFilter = FiltersStore.getInitialDropdownFilter(); | ||
|
||
/** The initial search filter value, not updated when search filter is changed */ | ||
initialSearchFilter: string = FiltersStore.getInitialSearchFilter(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why name it initial?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is named initial because this search filter variable is not updated at all when the user types. Instead this variable is simply used to initialize what is seen by the user at the search bar when they first open a new repo.
I have updated functionality to include the working logic for storing the status, type, sort, search bar, selected labels and hidden labels when the keep filters option is checked when switching repos. I also encountered a small bug where the DEFAULT_DROPDOWN_FILTER milestones property was being added to when switching repos since the array containing the milestones was passed by reference. My most recent push also fixes this issue. |
Update branch with latest updates to main
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm getting errors for structuredClone, is this necessary
ERROR in src/app/shared/issue-tables/FiltersStore.ts:8:58 - error TS2304: Cannot find name 'structuredClone'.
only available in node 17, we're using node 14
I'm not completely convinced that we need both initial and current variables, since by default it will reset, so why do we have to store the initial state? @CATcher-org/2223s2 what do you think? |
Summary:
Fixes #205
Type of change:
Changes Made:
Screenshots:
Proposed Commit Message:
Checklist: