-
Notifications
You must be signed in to change notification settings - Fork 3
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
fix(ui): add local storage to filters #1113
Changes from 8 commits
7ded6a2
83a3f01
4680a30
6a073d6
7016886
8c25cc1
b07bfe4
7c8992c
318c9d5
61402c9
f85485e
4a5b11d
f703158
f6ef122
24ffb73
4fe66ac
d92663c
29238a6
8b4d237
a13f4b2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,8 @@ | |
|
||
type FilterGroup = Partial<Record<opensearch.main.Field, C.DrawerFilterableGroup>>; | ||
|
||
export const FILTER_STORAGE_KEY = "osFilter"; | ||
|
||
export const useFilterState = () => { | ||
const { data: user } = useGetUser(); | ||
const url = useOsUrl(); | ||
|
@@ -74,6 +76,7 @@ | |
return (value: opensearch.FilterValue) => { | ||
setFilters((state) => { | ||
const updateState = { ...state, [field]: { ...state[field], value } }; | ||
// find all filter values to update | ||
const updateFilters = Object.values(updateState).filter((FIL) => { | ||
if (FIL.type === "terms") { | ||
const value = FIL.value as string[]; | ||
|
@@ -91,7 +94,12 @@ | |
|
||
return true; | ||
}); | ||
localStorage.setItem( | ||
FILTER_STORAGE_KEY, | ||
JSON.stringify({ filters: updateFilters, tab: url.state.tab }), | ||
); | ||
|
||
// this changes the tanstack query; which is used to query the data | ||
url.onSet((state) => ({ | ||
...state, | ||
filters: updateFilters, | ||
|
@@ -107,24 +115,53 @@ | |
setAccordionValues(updateAccordion); | ||
}; | ||
|
||
const onFilterReset = () => | ||
const onFilterReset = () => { | ||
url.onSet((s) => ({ | ||
...s, | ||
filters: [], | ||
pagination: { ...s.pagination, number: 0 }, | ||
})); | ||
localStorage.removeItem(FILTER_STORAGE_KEY); | ||
}; | ||
|
||
const filtersApplied = checkMultiFilter(url.state.filters, 1); | ||
|
||
// update initial filter state + accordion default open items | ||
// on filter initialization | ||
useEffect(() => { | ||
// check if any filters where saved in storage | ||
const filterStorage: string | null = localStorage.getItem(FILTER_STORAGE_KEY); | ||
if (!filterStorage) return; | ||
|
||
const filterState: { filters: C.DrawerFilterableGroup[]; tab: string } = | ||
JSON.parse(filterStorage); | ||
|
||
// we should delete the local storage if it doesn't match current tab | ||
if (filterState.tab !== url.state.tab) { | ||
localStorage.removeItem(FILTER_STORAGE_KEY); | ||
return; | ||
} | ||
|
||
// this changes the tanstack query; which is used to query the data | ||
url.onSet((state) => ({ | ||
...state, | ||
filters: filterState.filters, | ||
pagination: { ...state.pagination, number: 0 }, | ||
})); | ||
|
||
// eslint-disable-next-line | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please remove this comment There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was a very intensional comment, this useEffect should only run on first render. Which is why the use effect array is empty and should not be changed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What value does it expect? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it is expecting There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay Brian talked me into removing the comment, but I have added my own comment of why I have not put the values the linter suggests in the dependency array. Just so if someone wants to address these lint errors they can get a better idea of the whole picture. This sound cool to you? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that's a solid compromise. Will approve |
||
}, []); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's time to have a library do this for us. You can just make a file called I've seen @daniel-belcher's work with local storage and we keep repeating the same logic. The logic is complex and it doesn't help that these components now look so much more daunting than they need to be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm with it, but can we get with Erika to make a ticket, and point out the places we want to utilize this library/utility. Sounds like here, and then the code that Daniel is working with. All for it, I think it should be worked separately though only because I would like to whole sale replace it for Daniels code as well and in doing that we would then have to have it retested. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay so I am not going to move this work to a hook rn based off this comment^ |
||
|
||
// update filter display based on url query | ||
useEffect(() => { | ||
if (!drawer.drawerOpen) return; | ||
const updateAccordions = [...accordionValues] as any[]; | ||
|
||
setFilters((s) => { | ||
return Object.entries(s).reduce((STATE, [KEY, VAL]) => { | ||
const updateAccordions = [...accordionValues] as any[]; | ||
setFilters((currentFilters) => { | ||
// Set the new filters state based on the current filter data | ||
return Object.entries(currentFilters).reduce((STATE, [KEY, VAL]) => { | ||
const updateFilter = url.state.filters.find((FIL) => FIL.field === KEY); | ||
|
||
// Determine the new value for the filter based on the URL state | ||
const value = (() => { | ||
if (updateFilter) { | ||
updateAccordions.push(KEY); | ||
|
@@ -135,12 +172,14 @@ | |
return { gte: undefined, lte: undefined } as opensearch.RangeValue; | ||
})(); | ||
|
||
// Update the state with the new value for this filter | ||
STATE[KEY] = { ...VAL, value }; | ||
|
||
return STATE; | ||
}, {} as any); | ||
}); | ||
setAccordionValues(updateAccordions); | ||
}, [url.state.filters, drawer.drawerOpen]); | ||
|
||
const aggs = useMemo(() => { | ||
return Object.entries(_aggs || {}).reduce( | ||
|
@@ -157,7 +196,7 @@ | |
}, | ||
{} as Record<opensearch.main.Field, { label: string; value: string }[]>, | ||
); | ||
}, [_aggs]); | ||
}, [_aggs, labelMap]); | ||
|
||
return { | ||
aggs, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,10 @@ | ||
import { describe, expect, it } from "vitest"; | ||
import { describe, expect, it, vi, beforeEach, afterEach } from "vitest"; | ||
import { screen, within } from "@testing-library/react"; | ||
import userEvent from "@testing-library/user-event"; | ||
import { OsFilterDrawer } from "./index"; | ||
import { opensearch } from "shared-types"; | ||
import { renderFilterDrawer, getDashboardQueryString } from "@/utils/test-helpers"; | ||
import { FILTER_STORAGE_KEY } from "./index"; | ||
|
||
const setup = ( | ||
filters: opensearch.Filterable<opensearch.main.Field>[], | ||
|
@@ -21,6 +22,23 @@ const setup = ( | |
}; | ||
|
||
describe("OsFilterDrawer", () => { | ||
beforeEach(() => { | ||
global.localStorage = { | ||
setItem: vi.fn(), | ||
getItem: vi.fn(), | ||
removeItem: vi.fn(), | ||
key: vi.fn(), | ||
clear: vi.fn(), | ||
length: 0, | ||
}; | ||
|
||
vi.spyOn(global.localStorage, "getItem").mockReturnValue(null); | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if you store this spy in variable you wouldn't have to recreate it to change the value, for instance |
||
|
||
afterEach(() => { | ||
vi.clearAllMocks(); | ||
}); | ||
|
||
describe("SPA Filters", () => { | ||
it("should display the drawer closed initially", () => { | ||
setup([], "spas"); | ||
|
@@ -279,6 +297,7 @@ describe("OsFilterDrawer", () => { | |
|
||
const chip = screen.queryByLabelText("CHIP SPA"); | ||
expect(chip).toBeInTheDocument(); | ||
|
||
expect(chip.getAttribute("data-state")).toEqual("unchecked"); | ||
|
||
const med = screen.queryByLabelText("Medicaid SPA"); | ||
|
@@ -417,4 +436,69 @@ describe("OsFilterDrawer", () => { | |
expect(screen.getByRole("button", { name: "Reset" })).toBeInTheDocument(); | ||
expect(screen.getByRole("button", { name: "Close" })).toBeInTheDocument(); | ||
}); | ||
describe("localStorage", () => { | ||
it("should load filters from localStorage", async () => { | ||
const storedFilterState = [ | ||
{ | ||
label: "State", | ||
field: "state.keyword", | ||
component: "multiSelect", | ||
prefix: "must", | ||
type: "terms", | ||
value: ["MD"], | ||
}, | ||
]; | ||
vi.spyOn(global.localStorage, "getItem").mockReturnValue( | ||
JSON.stringify({ | ||
filters: storedFilterState, | ||
tab: "spas", | ||
}), | ||
); | ||
const { user } = setup([], "spas"); | ||
|
||
await user.click(screen.getByRole("button", { name: "Filters" })); | ||
|
||
const state = screen.getByRole("heading", { | ||
name: "State", | ||
level: 3, | ||
}).parentElement; | ||
expect(state.getAttribute("data-state")).toEqual("open"); | ||
|
||
const combo = screen.getByRole("combobox"); | ||
expect(combo).toBeInTheDocument(); | ||
expect(screen.queryByLabelText("Remove MD")).toBeInTheDocument(); | ||
}); | ||
|
||
it("should not set filters if the tab does not match", async () => { | ||
const storedFilterState = [ | ||
{ | ||
label: "State", | ||
field: "state.keyword", | ||
component: "multiSelect", | ||
prefix: "must", | ||
type: "terms", | ||
value: ["MD"], | ||
}, | ||
]; | ||
|
||
vi.spyOn(global.localStorage, "getItem").mockReturnValue( | ||
JSON.stringify({ | ||
filters: storedFilterState, | ||
tab: "spas", | ||
}), | ||
); | ||
|
||
const { user } = setup([], "waivers"); | ||
await user.click(screen.getByRole("button", { name: "Filters" })); | ||
|
||
expect(global.localStorage.removeItem).toHaveBeenCalledWith(FILTER_STORAGE_KEY); | ||
}); | ||
|
||
it("should do nothing if no filters are stored locally", async () => { | ||
vi.spyOn(global.localStorage, "getItem").mockReturnValue(null); | ||
const { user } = setup([], "spas"); | ||
await user.click(screen.getByRole("button", { name: "Filters" })); | ||
expect(global.localStorage.getItem).toHaveBeenCalledWith(FILTER_STORAGE_KEY); | ||
}); | ||
}); | ||
}); |
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.
this is old code that was removed I added back for this ticket: https://jiraent.cms.gov/browse/OY2-32770