From e2f18ccc1f86b3cf260d23bc60bc2d04c8e0fac4 Mon Sep 17 00:00:00 2001
From: Vera Kahn <verakahn@nypl.org>
Date: Wed, 8 Nov 2023 15:33:28 -0500
Subject: [PATCH] style fixes

---
 __test__/fixtures/testAggregations.js         | 129 +++++++++++++++++-
 pages/search/advanced.tsx                     |   4 +-
 .../ItemFilters/ItemFilterButtons.tsx         |   4 +-
 .../ItemFilters/ItemFilterLabel.tsx           |   4 +-
 .../ItemFilters/ItemFilters.test.tsx          |  95 ++++++-------
 src/utils/itemFilterUtils.ts                  |   9 +-
 src/utils/utilsTests/itemFilterUtils.test.tsx |   8 +-
 styles/components/ItemFilters.module.scss     |   4 +-
 8 files changed, 184 insertions(+), 73 deletions(-)

diff --git a/__test__/fixtures/testAggregations.js b/__test__/fixtures/testAggregations.js
index 7051400c7..f76108e37 100644
--- a/__test__/fixtures/testAggregations.js
+++ b/__test__/fixtures/testAggregations.js
@@ -1,4 +1,4 @@
-const aggs = [
+export const normalAggs = [
   {
     "@type": "nypl:Aggregation",
     "@id": "res:location",
@@ -80,4 +80,129 @@ const aggs = [
   },
 ]
 
-export default aggs
+export const aggsWithRepeatedValues = [
+  {
+    "@type": "nypl:Aggregation",
+    "@id": "res:location",
+    id: "location",
+    field: "location",
+    values: [
+      {
+        value: "loc:mym32",
+        count: 8,
+        label: "Performing Arts Research Collections - Music",
+      },
+    ],
+  },
+  {
+    "@type": "nypl:Aggregation",
+    "@id": "res:format",
+    id: "format",
+    field: "format",
+    values: [],
+  },
+  {
+    "@type": "nypl:Aggregation",
+    "@id": "res:status",
+    id: "status",
+    field: "status",
+    values: [
+      {
+        value: "status:a",
+        count: 4,
+        label: "Available",
+      },
+      {
+        value: "status:a",
+        count: 4,
+        label: "Available ",
+      },
+    ],
+  },
+]
+
+export const aggsWithMissingProperties = [
+  {
+    "@id": "res:location",
+    "@type": "nypl:Aggregation",
+    field: "location",
+    id: "location",
+    values: [
+      {
+        count: 4,
+        value: "loc:maj03",
+        label: "SASB M1 - General Research - Room 315",
+      },
+      {
+        count: 12,
+        label: "Offsite",
+        value: "loc:rc2ma",
+      },
+      {
+        count: 12,
+        label: "Off site",
+        value: "loc:rc2ma",
+      },
+      {
+        count: 12,
+        label: "Off-site",
+        value: "loc:rc2ma",
+      },
+      {
+        count: 12,
+        label: "off-site",
+        value: "loc:rc2ma",
+      },
+      {
+        count: 12,
+        label: "off site",
+        value: "loc:rc2ma",
+      },
+      {
+        count: 2,
+        value: "offsite",
+        label: "Offsite",
+      },
+      {
+        count: 2,
+        value: "blank",
+        label: "",
+      },
+      {
+        count: 2,
+        value: "blaaaank",
+      },
+    ],
+  },
+  {
+    "@id": "res:format",
+    "@type": "nypl:Aggregation",
+    field: "format",
+    id: "format",
+    values: [
+      {
+        count: 12,
+        label: "Text",
+        value: "Text",
+      },
+    ],
+  },
+  {
+    "@id": "res:status",
+    "@type": "nypl:Aggregation",
+    field: "status",
+    id: "status",
+    values: [
+      {
+        count: 12,
+        label: "Available",
+        value: "status:a",
+      },
+      {
+        count: 12,
+        label: "Not Available (ReCAP",
+        value: "status:na",
+      },
+    ],
+  },
+]
diff --git a/pages/search/advanced.tsx b/pages/search/advanced.tsx
index f5f2e8dd6..aebb66317 100644
--- a/pages/search/advanced.tsx
+++ b/pages/search/advanced.tsx
@@ -39,7 +39,7 @@ import type {
 import { getQueryString } from "../../src/utils/searchUtils"
 
 import ItemFilterContainer from "../../src/components/ItemFilters/FiltersContainer"
-import testItemAggs from "../../__test__/fixtures/testAggregations"
+import { normalAggs } from "../../__test__/fixtures/testAggregations"
 
 /**
  * The Advanced Search page is responsible for displaying the Advanced Search form fields and
@@ -125,7 +125,7 @@ export default function AdvancedSearch() {
             }
           />
         )}
-        <ItemFilterContainer itemAggs={testItemAggs} />
+        <ItemFilterContainer itemAggs={normalAggs} />
         <Heading level="two">Advanced Search</Heading>
         <Form
           id="advancedSearchForm"
diff --git a/src/components/ItemFilters/ItemFilterButtons.tsx b/src/components/ItemFilters/ItemFilterButtons.tsx
index 51a487497..f360fac57 100644
--- a/src/components/ItemFilters/ItemFilterButtons.tsx
+++ b/src/components/ItemFilters/ItemFilterButtons.tsx
@@ -42,14 +42,14 @@ const ItemFilterButtons = ({
         data-testid={`clear-${field}-button`}
         key={`clear-${field}-button`}
         buttonType="text"
-        id="clear-filter-button"
+        id={`clear-${field}-button`}
         onClick={clearFilter}
       >
         Clear
       </Button>
       <Button
         key={`apply-${field}-button`}
-        id="apply-filter-button"
+        id={`apply-${field}-button`}
         onClick={applyFilter}
       >
         Apply
diff --git a/src/components/ItemFilters/ItemFilterLabel.tsx b/src/components/ItemFilters/ItemFilterLabel.tsx
index be63ffcc4..96af2306f 100644
--- a/src/components/ItemFilters/ItemFilterLabel.tsx
+++ b/src/components/ItemFilters/ItemFilterLabel.tsx
@@ -1,7 +1,6 @@
 import { Button, Icon, Spacer } from "@nypl/design-system-react-components"
 import type { Dispatch } from "react"
 
-import styles from "../../../styles/components/ItemFilters.module.scss"
 import type { Option } from "../../types/filterTypes"
 
 interface ItemFilterLabelProps {
@@ -20,9 +19,8 @@ const ItemFilterLabel = ({
   const fieldFormatted = field[0].toUpperCase() + field.substring(1)
   return (
     <Button
-      className={styles.itemFilterButton}
       sx={{
-        borderColor: "var(--nypl-colors-ui-gray-medium)",
+        borderColor: "ui.gray.medium",
         color: "black",
         width: "100%",
       }}
diff --git a/src/components/ItemFilters/ItemFilters.test.tsx b/src/components/ItemFilters/ItemFilters.test.tsx
index d2df3a38c..0194364f3 100644
--- a/src/components/ItemFilters/ItemFilters.test.tsx
+++ b/src/components/ItemFilters/ItemFilters.test.tsx
@@ -5,7 +5,7 @@ import mockRouter from "next-router-mock"
 import FiltersContainer from "./FiltersContainer"
 import userEvent from "@testing-library/user-event"
 
-import aggs from "../../../__test__/fixtures/testAggregations"
+import { normalAggs } from "../../../__test__/fixtures/testAggregations"
 
 // Mock next router
 jest.mock("next/router", () => jest.requireActual("next-router-mock"))
@@ -16,12 +16,12 @@ describe("Filters container", () => {
   })
 
   it("renders a single filter", () => {
-    render(<FiltersContainer itemAggs={[aggs[0]]} />)
+    render(<FiltersContainer itemAggs={[normalAggs[0]]} />)
     const filters = screen.getAllByTestId(/item-filter/)
     expect(filters.length).toBe(1)
   })
   it("renders three filter boxes", () => {
-    render(<FiltersContainer itemAggs={aggs} />)
+    render(<FiltersContainer itemAggs={normalAggs} />)
     const filters = screen.getAllByTestId(/item-filter/)
     expect(filters.length).toBe(3)
   })
@@ -32,15 +32,12 @@ describe("Filters container", () => {
       item_format: "Text",
       item_status: "status:a",
     }
-    render(<FiltersContainer itemAggs={aggs} />)
-    const [
-      locationCheckboxGroupButton,
-      statusCheckboxGroupButton,
-      formatCheckboxGroupButton,
-    ] = filterButtons()
-    await filterHasSelected(locationCheckboxGroupButton, ["Offsite"])
-    await filterHasSelected(formatCheckboxGroupButton, ["Text"])
-    await filterHasSelected(statusCheckboxGroupButton, ["Available"])
+    render(<FiltersContainer itemAggs={normalAggs} />)
+    const { locationFilterButton, statusFilterButton, formatFilterButton } =
+      filterButtons()
+    await filterHasSelected(locationFilterButton, ["Offsite"])
+    await filterHasSelected(formatFilterButton, ["Text"])
+    await filterHasSelected(statusFilterButton, ["Available"])
   })
 
   it("clears selection per filter", async () => {
@@ -49,30 +46,24 @@ describe("Filters container", () => {
       item_format: "Text",
       item_status: "status:a,status:na",
     }
-    render(<FiltersContainer itemAggs={aggs} />)
-    const [
-      locationCheckboxGroupButton,
-      statusCheckboxGroupButton,
-      formatCheckboxGroupButton,
-    ] = filterButtons()
+    render(<FiltersContainer itemAggs={normalAggs} />)
+    const { locationFilterButton, statusFilterButton, formatFilterButton } =
+      filterButtons()
 
     // the values start selected
-    await filterHasSelected(locationCheckboxGroupButton, ["Offsite"])
-    await filterHasSelected(formatCheckboxGroupButton, ["Text"])
-    await filterHasSelected(statusCheckboxGroupButton, [
-      "Available",
-      "Not available",
-    ])
+    await filterHasSelected(locationFilterButton, ["Offsite"])
+    await filterHasSelected(formatFilterButton, ["Text"])
+    await filterHasSelected(statusFilterButton, ["Available", "Not available"])
     const clearStatusButton = screen.getByTestId("clear-status-button")
 
     await act(async () => {
       await userEvent.click(clearStatusButton)
       // these filters should be unchanged
-      await filterHasSelected(locationCheckboxGroupButton, ["Offsite"])
-      await filterHasSelected(formatCheckboxGroupButton, ["Text"])
+      await filterHasSelected(locationFilterButton, ["Offsite"])
+      await filterHasSelected(formatFilterButton, ["Text"])
 
       // Status values should be unchecked
-      await filterHasNotSelected(statusCheckboxGroupButton, [
+      await filterHasNotSelected(statusFilterButton, [
         "Available",
         "Not available",
       ])
@@ -80,33 +71,31 @@ describe("Filters container", () => {
   })
 
   it("does not persist selection if filter closes without applying", async () => {
-    render(<FiltersContainer itemAggs={aggs} />)
-    const [locationCheckboxGroupButton, statusCheckboxGroupButton] =
-      filterButtons()
+    render(<FiltersContainer itemAggs={normalAggs} />)
+    const { locationFilterButton, statusFilterButton } = filterButtons()
     await act(async () => {
-      await userEvent.click(locationCheckboxGroupButton)
+      await userEvent.click(locationFilterButton)
       const checkbox = screen.getAllByRole("checkbox")[0]
       await userEvent.click(checkbox)
 
       // clicking other filter label closes the location filter
-      await userEvent.click(statusCheckboxGroupButton)
-      await userEvent.click(locationCheckboxGroupButton)
+      await userEvent.click(statusFilterButton)
+      await userEvent.click(locationFilterButton)
       const applyButton = screen.getByTestId("clear-location-button")
       expect(applyButton).toBeDisabled()
     })
   })
 
-  it.only("persists previously applied selection after closing filter without applying", async () => {
+  it("persists previously applied selection after closing filter without applying", async () => {
     mockRouter.query = {
       item_location: "loc:rc2ma",
       item_format: "Text",
       item_status: "status:a,status:na",
     }
-    render(<FiltersContainer itemAggs={aggs} />)
-    const [locationCheckboxGroupButton, statusCheckboxGroupButton] =
-      filterButtons()
+    render(<FiltersContainer itemAggs={normalAggs} />)
+    const { locationFilterButton, statusFilterButton } = filterButtons()
     await act(async () => {
-      await userEvent.click(locationCheckboxGroupButton)
+      await userEvent.click(locationFilterButton)
       const checkbox = screen.getByLabelText(/Main Reading Room/)
       const offsiteCheckbox = screen.getByLabelText("Offsite")
       // uncheck Offsite (set from query parsing)
@@ -116,18 +105,18 @@ describe("Filters container", () => {
       // don't apply
 
       // clicking other filter label closes the location filter
-      await userEvent.click(statusCheckboxGroupButton)
-      await userEvent.click(locationCheckboxGroupButton)
+      await userEvent.click(statusFilterButton)
+      await userEvent.click(locationFilterButton)
 
-      filterHasSelected(locationCheckboxGroupButton, ["Offsite"])
-      filterHasNotSelected(locationCheckboxGroupButton, [
+      filterHasSelected(locationFilterButton, ["Offsite"])
+      filterHasNotSelected(locationFilterButton, [
         "Schwarzman Building - Main Reading Room 315",
       ])
     })
   })
   it("closes open filters when user clicks outside of the filter", async () => {
-    render(<FiltersContainer itemAggs={aggs} />)
-    const [locationFilterButton] = filterButtons()
+    render(<FiltersContainer itemAggs={normalAggs} />)
+    const { locationFilterButton } = filterButtons()
     const outsideOfTheFilter = screen.getByTestId("filter-text")
     await act(async () => {
       await userEvent.click(locationFilterButton)
@@ -152,15 +141,15 @@ const filterHasSelected = async (checkboxGroupButton, values: string[]) => {
 }
 
 const filterButtons = () => {
-  const locationCheckboxGroupButton = screen.getByTestId("location-item-filter")
-  const statusCheckboxGroupButton = screen.getByTestId("status-item-filter")
-  const formatCheckboxGroupButton = screen.getByTestId("format-item-filter")
-
-  return [
-    locationCheckboxGroupButton,
-    statusCheckboxGroupButton,
-    formatCheckboxGroupButton,
-  ]
+  const locationFilterButton = screen.getByTestId("location-item-filter")
+  const statusFilterButton = screen.getByTestId("status-item-filter")
+  const formatFilterButton = screen.getByTestId("format-item-filter")
+
+  return {
+    locationFilterButton,
+    statusFilterButton,
+    formatFilterButton,
+  }
 }
 
 const filterHasNotSelected = async (checkboxGroupButton, values: string[]) => {
diff --git a/src/utils/itemFilterUtils.ts b/src/utils/itemFilterUtils.ts
index 33b61d4b4..a5ad4a6b2 100644
--- a/src/utils/itemFilterUtils.ts
+++ b/src/utils/itemFilterUtils.ts
@@ -40,14 +40,13 @@ export const buildItemFilterQueryParams = (
     if (loc === "Offsite") return recapLocations
     else return loc
   })
-  const location_query = location.length
-    ? "item_location=" + locs.join(",")
-    : ""
+  const location_query = locs.length ? "item_location=" + locs.join(",") : ""
   const format_query = format.length ? "&item_format=" + format.join(",") : ""
   const status_query = status.length ? "&item_status=" + status.join(",") : ""
 
-  const query = encodeURI(`?${location_query}${format_query}${status_query}`)
-  return query.length > 3 ? query : ""
+  const query = `${location_query}${format_query}${status_query}`
+  if (query.length) return encodeURI("?" + query)
+  else return ""
 }
 
 export const buildAppliedFiltersString = (
diff --git a/src/utils/utilsTests/itemFilterUtils.test.tsx b/src/utils/utilsTests/itemFilterUtils.test.tsx
index c4b6152ce..4beabecfa 100644
--- a/src/utils/utilsTests/itemFilterUtils.test.tsx
+++ b/src/utils/utilsTests/itemFilterUtils.test.tsx
@@ -5,7 +5,7 @@ import {
   buildItemFilterQueryParams,
   buildAppliedFiltersString,
 } from "../itemFilterUtils"
-import testAggregations from "../../../__test__/fixtures/testAggregations"
+import { normalAggs } from "../../../__test__/fixtures/testAggregations"
 import { ItemFilterData } from "../../models/itemFilterData"
 
 describe("Item Filter Utils", () => {
@@ -13,7 +13,7 @@ describe("Item Filter Utils", () => {
     it("returns true for a recap location", () => {
       expect(isRecapLocation("loc:rc2ma")).toBe(true)
     })
-    it("returns false for a recap location", () => {
+    it("returns false for a non-recap location", () => {
       expect(isRecapLocation("loc:xc")).toBe(false)
     })
   })
@@ -34,7 +34,7 @@ describe("Item Filter Utils", () => {
       const locations = ["loc:rc2ma", "loc:rcma2", "loc:rcrc"]
       expect(combineRecapLocations(locations)).toEqual(["Offsite"])
     })
-    it("can handle an empty array", () => {
+    it("can handle no locations", () => {
       const locations = []
       expect(combineRecapLocations(locations)).toEqual([])
     })
@@ -96,7 +96,7 @@ describe("Item Filter Utils", () => {
       item_status: "status:a",
       item_format: "Text",
     }
-    const aggs = testAggregations.map((agg) => new ItemFilterData(agg))
+    const aggs = normalAggs.map((agg) => new ItemFilterData(agg))
     it("can handle no filters", () => {
       expect(buildAppliedFiltersString({}, 30, [])).toBe("30 Items")
     })
diff --git a/styles/components/ItemFilters.module.scss b/styles/components/ItemFilters.module.scss
index 26f2a48cb..f12efc064 100644
--- a/styles/components/ItemFilters.module.scss
+++ b/styles/components/ItemFilters.module.scss
@@ -24,7 +24,7 @@
 
 .itemFilter {
   background-color: white;
-  width: 125px;
+  width: 230px;
   display: inline-block;
-  margin-right: var(--nypl-space-xs);
+  margin-right: var(--nypl-space-s);
 }