Skip to content
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

feat: only allow one filter open at a time, easy closing #3084

Merged
merged 50 commits into from
Jan 3, 2025
Merged
Show file tree
Hide file tree
Changes from 46 commits
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
5789498
first pass, back-end
angelathe Dec 13, 2024
f6db9d7
first pass, front-end
angelathe Dec 13, 2024
502ec96
simplify date range query, dont need subquery
angelathe Dec 16, 2024
f56eabc
update and add tests
angelathe Dec 16, 2024
23d6171
Merge branch 'main' into angela/2752-filter-by-date-p1
angelathe Dec 16, 2024
aa66bf0
make filtersService file, abstract components
angelathe Dec 17, 2024
7fdac87
add test for convertDateOptionToDateRange
angelathe Dec 17, 2024
3a72349
update date helpers, modify tests
angelathe Dec 19, 2024
8110125
wip, Filters
angelathe Dec 19, 2024
c5b617e
update local dev default to last year
angelathe Dec 19, 2024
1481bbe
Merge branch 'main' into angela/2752-filter-by-date-p1
angelathe Dec 19, 2024
f443b01
udpate tests to reflect local dev default
angelathe Dec 19, 2024
d81ba81
else if nit
angelathe Dec 19, 2024
bfd8fbf
refactor: moar abstraction (#3078)
mcmcgrath13 Dec 19, 2024
bbe6233
refactor Filters file to add BaseFilter file
angelathe Dec 20, 2024
235bf63
add BaseFilter
angelathe Dec 20, 2024
2d495cf
fix todays date for test
angelathe Dec 20, 2024
1f09869
move FiltersService to utils as date-utils
angelathe Dec 20, 2024
3c82083
add test to check reading query will result in correct date range
angelathe Dec 20, 2024
50bdae1
Update dateRangelabels
angelathe Dec 20, 2024
b12546b
[pre-commit.ci] auto fixes from pre-commit hooks
pre-commit-ci[bot] Dec 20, 2024
11bcceb
reverting change temporarily, causing test failures
angelathe Dec 20, 2024
d60ef8f
update default nits
angelathe Dec 20, 2024
59168ec
update default nits
angelathe Dec 20, 2024
3e6d179
hardcode expected start dates in tests
angelathe Dec 20, 2024
5fee934
Flexible widths
angelathe Dec 23, 2024
6ea3832
fix small nit, tag disappeared when no conditions were selected
angelathe Dec 23, 2024
ec50452
update snapshot test for tag, turn btnClass to functional prop isActive
angelathe Dec 23, 2024
068a36d
refactor dateRangeLabels
angelathe Dec 23, 2024
0c2c781
move default date range const to date-utils because forgot to import …
angelathe Dec 23, 2024
72cfbc8
update docstrings
angelathe Dec 23, 2024
1764ca5
refactor updateQueryParam
angelathe Dec 23, 2024
bf3631f
update today test fixture to add time
angelathe Dec 23, 2024
5fa2a05
Merge branch 'main' into angela/2752-filter-by-date-p1
angelathe Dec 23, 2024
fcd642f
feat: only allow one filter open at a time, easy closing
mcmcgrath13 Dec 23, 2024
032fe1d
fix: merge in upstream
mcmcgrath13 Dec 23, 2024
eec5273
Update containers/ecr-viewer/src/app/components/BaseFilter.tsx
mcmcgrath13 Dec 23, 2024
0a39c61
fix: reset trigger
mcmcgrath13 Dec 23, 2024
339bc4b
fix: cleanup
mcmcgrath13 Dec 23, 2024
3a097b5
fix: only listen when a filter is open
mcmcgrath13 Dec 23, 2024
4665c0e
fix: consolidate trigger logic, add tests
mcmcgrath13 Dec 23, 2024
501f42d
docs: comment
mcmcgrath13 Dec 23, 2024
a48d94b
fix: comments, cleanup
mcmcgrath13 Dec 23, 2024
897c03d
fix: moar =
mcmcgrath13 Dec 23, 2024
227891b
fix: merge in main
mcmcgrath13 Dec 23, 2024
f535b28
test: focus handling
mcmcgrath13 Dec 23, 2024
57ccfe7
refactor: use imports
mcmcgrath13 Jan 2, 2025
af731d1
refactor: maybe better readability
mcmcgrath13 Jan 2, 2025
49f75ad
fix: appease typescript
mcmcgrath13 Jan 2, 2025
620937e
fix: tie up loose ends
mcmcgrath13 Jan 2, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 27 additions & 7 deletions containers/ecr-viewer/src/app/components/BaseFilter.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import React, { ComponentType, ReactNode, useCallback, useState } from "react";
import React, { ComponentType, ReactNode, useCallback } from "react";
import { Button } from "@trussworks/react-uswds";
import { useRouter, usePathname, useSearchParams } from "next/navigation";
import { FilterOpenContext } from "./Filters";

/**
* Custom hook to manage query parameters in the URL (set, delete, and update). Hook always resets page back to 1.
Expand Down Expand Up @@ -94,21 +95,40 @@ export const Filter = ({
submitHandler: () => void;
children: ReactNode;
}) => {
const [isFilterBoxOpen, setIsFilterBoxOpen] = useState(false);
const { filterBoxOpen, setFilterBoxOpen, lastOpenButtonRef } =
React.useContext(FilterOpenContext);
const openBtnRef = React.useRef<HTMLElement | null>(null);

const isFilterBoxOpen = filterBoxOpen === type;
const setIsFilterBoxOpen = React.useCallback((open: boolean) => {
if (open) {
setFilterBoxOpen(type);
// Set the last open button to this button when we open it
lastOpenButtonRef.current = openBtnRef.current?.parentElement || null;
} else {
setFilterBoxOpen(null);
}
openBtnRef?.current?.parentElement?.focus();
}, []);

// This filter has closed. We need the special submitted case to prevent
// a race condition with submitting and resetting if we try to do a reset
// just after submitting.
React.useEffect(() => {
if (filterBoxOpen !== "__submitted__" && filterBoxOpen !== type) {
resetHandler();
}
}, [filterBoxOpen]);

return (
<div>
<div onClick={(e) => e.stopPropagation()}>
<div className="position-relative display-flex flex-column">
<Button
className={`margin-right-0 ${isActive ? "filters-applied" : "filter-button"}`}
aria-label={`Filter by ${type}`}
aria-haspopup="listbox"
aria-expanded={isFilterBoxOpen}
onClick={() => {
if (isFilterBoxOpen) {
resetHandler();
}
setIsFilterBoxOpen(!isFilterBoxOpen);
}}
type="button"
Expand All @@ -133,7 +153,7 @@ export const Filter = ({
onSubmit={(e) => {
e.preventDefault();
submitHandler();
setIsFilterBoxOpen(false);
setFilterBoxOpen("__submitted__");
openBtnRef?.current?.parentElement?.focus();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we want it to set filterBoxOpen to "__submitted__"? It's slightly unclear how filterBoxOpen gets used when it can equal either filter type or "__submitted__"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's a tricky race condition this helps us avoid. There's a comment on line 121 about it. But the problem boils down to the resetting relying on the params being correct and on submit the params don't update until the next tic, so if we go the null state and reset, then we reset to the old state and not the new. It feels hacky, but I haven't thought of a better way yet 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I refactored a little to hopefully maybe improve the readability a bit, but it's still fundamentally the same approach 😞

}}
>
Expand Down
55 changes: 53 additions & 2 deletions containers/ecr-viewer/src/app/components/Filters.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,19 +9,70 @@ import {
dateRangeLabels,
} from "@/app/view-data/utils/date-utils";

// We use a context to communicate between the overall <Filters /> component
// and the `<Filter />` component to avoid prop drilling
type FilterOpenContextValue = {
filterBoxOpen: string | null;
setFilterBoxOpen: (v: string | null) => void;
lastOpenButtonRef: { current: HTMLElement | null };
};

export const FilterOpenContext = React.createContext<FilterOpenContextValue>({
filterBoxOpen: null,
setFilterBoxOpen: () => {},
lastOpenButtonRef: { current: null },
});

/**
* Functional component that renders Filters section in eCR Library.
* Includes Filter component for reportable conditions.
* @returns The rendered Filters component.
*/
const Filters = () => {
const [filterBoxOpen, setFilterBoxOpen] = useState<string | null>(null);
const lastOpenButtonRef = React.useRef<HTMLElement | null>(null);

const filterOpenContextValue = {
filterBoxOpen,
setFilterBoxOpen,
lastOpenButtonRef,
};

const resetFilters = React.useCallback(() => {
setFilterBoxOpen(null);
}, []);

// When a filter is open, close it if the escape key is hit or a click happens
// outside the <Filter /> component (implemented by stopping click propogation on <Filter />)
React.useEffect(() => {
angelathe marked this conversation as resolved.
Show resolved Hide resolved
const handleEscapeFilters = (event: KeyboardEvent) => {
if (event.code === "Escape") {
resetFilters();

// Return focus to the most recently selected open button
lastOpenButtonRef.current?.focus();
lastOpenButtonRef.current = null;
}
};
if (filterBoxOpen) {
window.addEventListener("keydown", handleEscapeFilters);
window.addEventListener("click", resetFilters);
return () => {
angelathe marked this conversation as resolved.
Show resolved Hide resolved
window.removeEventListener("keydown", handleEscapeFilters);
window.removeEventListener("click", resetFilters);
};
}
}, [filterBoxOpen]);

return (
<div>
<div className="border-top border-base-lighter"></div>
<div className="margin-x-3 margin-y-105 display-flex flex-align-center gap-105">
<span className="line-height-sans-6">FILTERS:</span>
<FilterByDate />
<FilterReportableConditions />
<FilterOpenContext.Provider value={filterOpenContextValue}>
<FilterByDate />
<FilterReportableConditions />
</FilterOpenContext.Provider>
</div>
</div>
);
Expand Down
128 changes: 128 additions & 0 deletions containers/ecr-viewer/src/app/tests/components/Filters.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,8 @@ describe("Filter by Reportable Conditions Component", () => {
const applyButton = screen.getByRole("button", { name: /Apply Filter/i });
fireEvent.click(applyButton);

expect(toggleFilterButton).toHaveFocus();

// Should have other condition in search param
expect(mockPush).toHaveBeenCalledWith(
expect.stringContaining("condition=Condition2"),
Expand Down Expand Up @@ -385,6 +387,8 @@ describe("Filter by Date Component", () => {
const applyButton = screen.getByRole("button", { name: /Apply Filter/i });
fireEvent.click(applyButton);

expect(toggleButton).toHaveFocus();

// Should have date range in search param
expect(mockPush).toHaveBeenCalledWith(
expect.stringContaining("dateRange=last-7-days"),
Expand Down Expand Up @@ -420,3 +424,127 @@ describe("Filter by Date Component", () => {
expect(radioAfterReset).not.toBeChecked();
});
});

describe("Filter Opening/Closing Controls", () => {
it("If a date range is checked but escape is hit without applying filter, filters should reset", async () => {
render(<Filters />);
const toggleButton = screen.getByRole("button", {
name: /Filter by Received Date/i,
});
fireEvent.click(toggleButton);

// Click different date option, but user closes button before applying filter
const radio = await waitFor(() =>
screen.getByRole("radio", {
name: "Last 7 days",
}),
);
fireEvent.click(radio);
expect(radio).toBeChecked();

fireEvent.keyDown(window, { code: "Escape" });

// should be closed
expect(screen.queryByRole("radio")).not.toBeInTheDocument();

// Selection should not persist because filter was not applied
expect(screen.getByText("Last year")).toBeInTheDocument();

// Focus should reset
expect(toggleButton).toHaveFocus();

// open and check reset
fireEvent.click(toggleButton);
const radioAfterReset = await waitFor(() =>
screen.getByRole("radio", {
name: "Last 7 days",
}),
);
expect(radioAfterReset).not.toBeChecked();
});

it("If a date range is checked but outside click is hit without applying filter, filters should reset", async () => {
render(
<div data-testid="outside">
<Filters />
</div>,
);
const toggleButton = screen.getByRole("button", {
name: /Filter by Received Date/i,
});
fireEvent.click(toggleButton);

// Click different date option, but user closes button before applying filter
const radio = await waitFor(() =>
screen.getByRole("radio", {
name: "Last 7 days",
}),
);
fireEvent.click(radio);
expect(radio).toBeChecked();

const outsideDiv = screen.getByTestId("outside");
fireEvent.click(outsideDiv);

// should be closed
expect(screen.queryByRole("radio")).not.toBeInTheDocument();

// Selection should not persist because filter was not applied
expect(screen.getByText("Last year")).toBeInTheDocument();

// open and check reset
fireEvent.click(toggleButton);
const radioAfterReset = await waitFor(() =>
screen.getByRole("radio", {
name: "Last 7 days",
}),
);
expect(radioAfterReset).not.toBeChecked();
});

it("If a date range is checked but condition button is hit, date should reset and close", async () => {
render(<Filters />);
const dateToggleButton = screen.getByRole("button", {
name: /Filter by Received Date/i,
});
fireEvent.click(dateToggleButton);

// Click different date option, but user closes button before applying filter
const radio = await waitFor(() =>
screen.getByRole("radio", {
name: "Last 7 days",
}),
);
fireEvent.click(radio);
expect(radio).toBeChecked();

const conditionToggleButton = screen.getByRole("button", {
name: /Filter by reportable condition/i,
});

fireEvent.click(conditionToggleButton);

// date should be closed
expect(screen.queryByRole("radio")).not.toBeInTheDocument();

// Selection should not persist because filter was not applied
expect(screen.getByText("Last year")).toBeInTheDocument();

// condtion should be open
expect(
screen.getByText("Filter by Reportable Condition"),
).toBeInTheDocument();

// open date and check reset
fireEvent.click(dateToggleButton);
const radioAfterReset = await waitFor(() =>
screen.getByRole("radio", {
name: "Last 7 days",
}),
);
expect(radioAfterReset).not.toBeChecked();

// condtion should be closed
expect(screen.queryByText("Select all")).not.toBeInTheDocument();
});
});
Loading