From da45ee5e7e1df8d0f51afeb6cd39745a638c4bf5 Mon Sep 17 00:00:00 2001 From: Kris McGinnes Date: Thu, 11 Jun 2026 15:30:28 -0500 Subject: [PATCH 1/3] Add paging to the icon picker Replace the icon picker's hard 64-icon cap and "type to search" hint with paging through the full icon list. Adds a minimal footer (Page X of Y plus prev/next chevrons) modeled on the query results list, 64 icons per page. Page position persists while the style dialog stays open and resets on search or when the dialog reopens. --- .../src/components/IconPicker.test.tsx | 196 +++++++++++++++--- .../src/components/IconPicker.tsx | 88 ++++++-- vitest.config.ts | 8 +- 3 files changed, 238 insertions(+), 54 deletions(-) diff --git a/packages/graph-explorer/src/components/IconPicker.test.tsx b/packages/graph-explorer/src/components/IconPicker.test.tsx index 1018ff82e..4d33c6a9f 100644 --- a/packages/graph-explorer/src/components/IconPicker.test.tsx +++ b/packages/graph-explorer/src/components/IconPicker.test.tsx @@ -4,6 +4,18 @@ import userEvent from "@testing-library/user-event"; import { IconPicker } from "./IconPicker"; +function visibleIconButtons() { + return screen.getAllByRole("button").filter(btn => btn.title !== ""); +} + +function firstVisibleIcon() { + return visibleIconButtons()[0]; +} + +function visibleIconNames() { + return visibleIconButtons().map(btn => btn.title); +} + describe("IconPicker", () => { it("should render Browse button", () => { render(); @@ -27,10 +39,7 @@ describe("IconPicker", () => { // Wait for at least some icon buttons to appear in the grid await waitFor(() => { - const iconButtons = screen - .getAllByRole("button") - .filter(btn => btn.title && btn.title !== ""); - expect(iconButtons.length).toBeGreaterThan(0); + expect(visibleIconButtons().length).toBeGreaterThan(0); }); }); @@ -44,23 +53,23 @@ describe("IconPicker", () => { await user.type(searchInput, "user"); await waitFor(() => { - const iconButtons = screen - .getAllByRole("button") - .filter(btn => btn.title && btn.title.includes("user")); - expect(iconButtons.length).toBeGreaterThan(0); + const matching = visibleIconButtons().filter(btn => + btn.title.includes("user"), + ); + expect(matching.length).toBeGreaterThan(0); }); }); - it("should show truncation hint when results are capped", async () => { + it("should show the pager when the icons span more than one page", async () => { const user = userEvent.setup(); render(); await user.click(screen.getByRole("button", { name: /browse/i })); - expect(screen.getByText(/Showing 64 of/)).toBeInTheDocument(); + expect(screen.getByText(/Page 1 of/)).toBeInTheDocument(); }); - it("should hide truncation hint when fewer results than cap", async () => { + it("should hide the pager when the results fit on one page", async () => { const user = userEvent.setup(); render(); @@ -69,7 +78,147 @@ describe("IconPicker", () => { await user.type(searchInput, "airplay"); - expect(screen.queryByText(/Showing 64 of/)).not.toBeInTheDocument(); + expect(screen.queryByText(/Page 1 of/)).not.toBeInTheDocument(); + }); + + it("should advance to the next page and show different icons", async () => { + const user = userEvent.setup(); + render(); + + await user.click(screen.getByRole("button", { name: /browse/i })); + const firstPageIcons = visibleIconNames(); + + await user.click(screen.getByRole("button", { name: "Next page" })); + + expect(screen.getByText(/Page 2 of/)).toBeInTheDocument(); + expect(visibleIconNames()).not.toEqual(firstPageIcons); + }); + + it("should disable Previous on the first page", async () => { + const user = userEvent.setup(); + render(); + + await user.click(screen.getByRole("button", { name: /browse/i })); + + expect( + screen.getByRole("button", { name: "Previous page" }), + ).toBeDisabled(); + expect( + screen.getByRole("button", { name: "Next page" }), + ).not.toBeDisabled(); + }); + + it("should disable Next on the last page", async () => { + const user = userEvent.setup(); + render(); + + await user.click(screen.getByRole("button", { name: /browse/i })); + // "arrow" matches enough icons to fill exactly two pages. + await user.type(screen.getByPlaceholderText("Search icons..."), "arrow"); + await user.click(screen.getByRole("button", { name: "Next page" })); + + expect(screen.getByText(/Page 2 of 2/)).toBeInTheDocument(); + expect(screen.getByRole("button", { name: "Next page" })).toBeDisabled(); + expect( + screen.getByRole("button", { name: "Previous page" }), + ).not.toBeDisabled(); + }); + + it("should return to the first page when the search changes", async () => { + const user = userEvent.setup(); + render(); + + await user.click(screen.getByRole("button", { name: /browse/i })); + await user.click(screen.getByRole("button", { name: "Next page" })); + expect(screen.getByText(/Page 2 of/)).toBeInTheDocument(); + + await user.type(screen.getByPlaceholderText("Search icons..."), "a"); + + expect(screen.getByText(/Page 1 of/)).toBeInTheDocument(); + }); + + it("should clear the search when the popover is reopened", async () => { + const user = userEvent.setup(); + render(); + + await user.click(screen.getByRole("button", { name: /browse/i })); + await user.type(screen.getByPlaceholderText("Search icons..."), "airplay"); + + // Close by pressing Escape, then reopen. + await user.keyboard("{Escape}"); + await waitFor(() => { + expect( + screen.queryByPlaceholderText("Search icons..."), + ).not.toBeInTheDocument(); + }); + await user.click(screen.getByRole("button", { name: /browse/i })); + + expect(screen.getByPlaceholderText("Search icons...")).toHaveValue(""); + }); + + it("should always open on the first page, even with a selection on a later page", async () => { + const user = userEvent.setup(); + // "zap" sorts near the end of the alphabetised list, well past page 1. + render(); + + await user.click(screen.getByRole("button", { name: /browse/i })); + + expect(screen.getByText(/Page 1 of/)).toBeInTheDocument(); + }); + + it("should reopen on the same page after closing with Escape", async () => { + const user = userEvent.setup(); + render(); + + await user.click(screen.getByRole("button", { name: /browse/i })); + await user.click(screen.getByRole("button", { name: "Next page" })); + expect(screen.getByText(/Page 2 of/)).toBeInTheDocument(); + + await user.keyboard("{Escape}"); + await waitFor(() => { + expect( + screen.queryByPlaceholderText("Search icons..."), + ).not.toBeInTheDocument(); + }); + await user.click(screen.getByRole("button", { name: /browse/i })); + + expect(screen.getByText(/Page 2 of/)).toBeInTheDocument(); + }); + + it("should reset to the first page when the component is remounted", async () => { + const user = userEvent.setup(); + // Remounting mirrors the node style dialog closing and reopening, which + // unmounts the picker and discards its page state. + const { unmount } = render(); + + await user.click(screen.getByRole("button", { name: /browse/i })); + await user.click(screen.getByRole("button", { name: "Next page" })); + expect(screen.getByText(/Page 2 of/)).toBeInTheDocument(); + + unmount(); + render(); + await user.click(screen.getByRole("button", { name: /browse/i })); + + expect(screen.getByText(/Page 1 of/)).toBeInTheDocument(); + }); + + it("should reopen on the same page after selecting an icon", async () => { + const user = userEvent.setup(); + render(); + + await user.click(screen.getByRole("button", { name: /browse/i })); + await user.click(screen.getByRole("button", { name: "Next page" })); + expect(screen.getByText(/Page 2 of/)).toBeInTheDocument(); + + await user.click(firstVisibleIcon()); + await waitFor(() => { + expect( + screen.queryByPlaceholderText("Search icons..."), + ).not.toBeInTheDocument(); + }); + await user.click(screen.getByRole("button", { name: /browse/i })); + + expect(screen.getByText(/Page 2 of/)).toBeInTheDocument(); }); it("should show no results message for invalid search", async () => { @@ -92,15 +241,10 @@ describe("IconPicker", () => { await user.click(screen.getByRole("button", { name: /browse/i })); await waitFor(() => { - const iconButtons = screen - .getAllByRole("button") - .filter(btn => btn.title && btn.title !== ""); - expect(iconButtons.length).toBeGreaterThan(0); + expect(visibleIconButtons().length).toBeGreaterThan(0); }); - const firstIcon = screen - .getAllByRole("button") - .filter(btn => btn.title && btn.title !== "")[0]; + const firstIcon = firstVisibleIcon(); const iconName = firstIcon.getAttribute("title"); await user.click(firstIcon); @@ -137,9 +281,7 @@ describe("IconPicker", () => { await user.click(screen.getByRole("button", { name: /browse/i })); await waitFor(() => { - const iconButtons = screen - .getAllByRole("button") - .filter(btn => btn.title && btn.title !== ""); + const iconButtons = visibleIconButtons(); expect(iconButtons.length).toBeGreaterThan(0); for (const btn of iconButtons) { expect(btn).toHaveAttribute("aria-pressed", "false"); @@ -154,16 +296,10 @@ describe("IconPicker", () => { await user.click(screen.getByRole("button", { name: /browse/i })); await waitFor(() => { - const iconButtons = screen - .getAllByRole("button") - .filter(btn => btn.title && btn.title !== ""); - expect(iconButtons.length).toBeGreaterThan(0); + expect(visibleIconButtons().length).toBeGreaterThan(0); }); - const firstIcon = screen - .getAllByRole("button") - .filter(btn => btn.title && btn.title !== "")[0]; - await user.click(firstIcon); + await user.click(firstVisibleIcon()); await waitFor(() => { expect( diff --git a/packages/graph-explorer/src/components/IconPicker.tsx b/packages/graph-explorer/src/components/IconPicker.tsx index 724f82225..0517b6556 100644 --- a/packages/graph-explorer/src/components/IconPicker.tsx +++ b/packages/graph-explorer/src/components/IconPicker.tsx @@ -1,4 +1,4 @@ -import { SearchIcon } from "lucide-react"; +import { ChevronLeftIcon, ChevronRightIcon, SearchIcon } from "lucide-react"; import { DynamicIcon } from "lucide-react/dynamic"; import { useState } from "react"; @@ -21,7 +21,7 @@ import { PopoverTrigger, } from "."; -const MAX_VISIBLE = 64; +const PAGE_SIZE = 64; export function IconPicker({ currentIconUrl, @@ -41,9 +41,25 @@ export function IconPicker({ }) { const [open, setOpen] = useState(false); const [search, setSearch] = useState(""); + const [page, setPage] = useState(0); const selectedName = getLucideName(currentIconUrl); const filtered = filterIcons(search); + const pageCount = Math.ceil(filtered.length / PAGE_SIZE); + const pageStart = page * PAGE_SIZE; + const pageRows = filtered.slice(pageStart, pageStart + PAGE_SIZE); + + function handleOpenChange(isOpen: boolean) { + setOpen(isOpen); + if (!isOpen) { + setSearch(""); + } + } + + function handleSearchChange(value: string) { + setSearch(value); + setPage(0); + } function handleSelect(iconName: IconName) { onSelect(toLucideIconRef(iconName), "image/svg+xml"); @@ -52,7 +68,7 @@ export function IconPicker({ } return ( - + + + + + ); +} + function IconButton({ name, selected, @@ -124,14 +179,7 @@ function IconButton({ } function filterIcons(search: string) { - if (!search) return allIconNamesSorted.slice(0, MAX_VISIBLE); + if (!search) return allIconNamesSorted; const lower = search.toLowerCase(); - const results: IconName[] = []; - for (const name of allIconNamesSorted) { - if (name.includes(lower)) { - results.push(name); - if (results.length >= MAX_VISIBLE) break; - } - } - return results; + return allIconNamesSorted.filter(name => name.includes(lower)); } diff --git a/vitest.config.ts b/vitest.config.ts index 34c3e30c1..eec1bff4e 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -6,10 +6,10 @@ export default defineConfig({ coverage: { thresholds: { autoUpdate: (newThreshold: number) => Math.floor(newThreshold), - statements: 64, - branches: 44, - functions: 58, - lines: 72, + statements: 65, + branches: 45, + functions: 60, + lines: 73, }, }, }, From 8ff0a9cc851c43e0d4ad9acb6c836f6ee96d94a7 Mon Sep 17 00:00:00 2001 From: Kris McGinnes Date: Thu, 11 Jun 2026 16:21:57 -0500 Subject: [PATCH 2/3] Use Tooltip component for icon picker icons Replace the native title attribute on each icon button with the Button tooltip prop, giving a styled tooltip and an aria-label accessible name. Tests identify icon buttons by aria-pressed and read names from aria-label, and wrap renders in TooltipProvider as the app does. --- .../src/components/IconPicker.test.tsx | 84 ++++++++++--------- .../src/components/IconPicker.tsx | 2 +- 2 files changed, 47 insertions(+), 39 deletions(-) diff --git a/packages/graph-explorer/src/components/IconPicker.test.tsx b/packages/graph-explorer/src/components/IconPicker.test.tsx index 4d33c6a9f..171fdd9c7 100644 --- a/packages/graph-explorer/src/components/IconPicker.test.tsx +++ b/packages/graph-explorer/src/components/IconPicker.test.tsx @@ -1,30 +1,49 @@ // @vitest-environment happy-dom +import type { ComponentProps } from "react"; + import { render, screen, waitFor } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import { IconPicker } from "./IconPicker"; +import { TooltipProvider } from "./Tooltip"; + +// The icon tooltips require a TooltipProvider ancestor, which the app supplies +// globally in DefaultLayout. +function renderPicker(props: Partial> = {}) { + return render(, { + wrapper: TooltipProvider, + }); +} +// Icon buttons are the only buttons in the picker with `aria-pressed`, and +// their tooltip surfaces the icon name as the accessible name (aria-label). function visibleIconButtons() { - return screen.getAllByRole("button").filter(btn => btn.title !== ""); + return screen + .getAllByRole("button") + .filter(btn => btn.hasAttribute("aria-pressed")); } function firstVisibleIcon() { return visibleIconButtons()[0]; } +function iconName(btn: HTMLElement) { + return btn.getAttribute("aria-label") ?? ""; +} + function visibleIconNames() { - return visibleIconButtons().map(btn => btn.title); + return visibleIconButtons().map(iconName); } describe("IconPicker", () => { it("should render Browse button", () => { - render(); + renderPicker(); expect(screen.getByRole("button", { name: /browse/i })).toBeInTheDocument(); }); it("should open popover with search input on click", async () => { const user = userEvent.setup(); - render(); + renderPicker(); await user.click(screen.getByRole("button", { name: /browse/i })); @@ -33,7 +52,7 @@ describe("IconPicker", () => { it("should show icons in the grid", async () => { const user = userEvent.setup(); - render(); + renderPicker(); await user.click(screen.getByRole("button", { name: /browse/i })); @@ -45,7 +64,7 @@ describe("IconPicker", () => { it("should filter icons when searching", async () => { const user = userEvent.setup(); - render(); + renderPicker(); await user.click(screen.getByRole("button", { name: /browse/i })); const searchInput = screen.getByPlaceholderText("Search icons..."); @@ -54,7 +73,7 @@ describe("IconPicker", () => { await waitFor(() => { const matching = visibleIconButtons().filter(btn => - btn.title.includes("user"), + iconName(btn).includes("user"), ); expect(matching.length).toBeGreaterThan(0); }); @@ -62,7 +81,7 @@ describe("IconPicker", () => { it("should show the pager when the icons span more than one page", async () => { const user = userEvent.setup(); - render(); + renderPicker(); await user.click(screen.getByRole("button", { name: /browse/i })); @@ -71,7 +90,7 @@ describe("IconPicker", () => { it("should hide the pager when the results fit on one page", async () => { const user = userEvent.setup(); - render(); + renderPicker(); await user.click(screen.getByRole("button", { name: /browse/i })); const searchInput = screen.getByPlaceholderText("Search icons..."); @@ -83,7 +102,7 @@ describe("IconPicker", () => { it("should advance to the next page and show different icons", async () => { const user = userEvent.setup(); - render(); + renderPicker(); await user.click(screen.getByRole("button", { name: /browse/i })); const firstPageIcons = visibleIconNames(); @@ -96,7 +115,7 @@ describe("IconPicker", () => { it("should disable Previous on the first page", async () => { const user = userEvent.setup(); - render(); + renderPicker(); await user.click(screen.getByRole("button", { name: /browse/i })); @@ -110,7 +129,7 @@ describe("IconPicker", () => { it("should disable Next on the last page", async () => { const user = userEvent.setup(); - render(); + renderPicker(); await user.click(screen.getByRole("button", { name: /browse/i })); // "arrow" matches enough icons to fill exactly two pages. @@ -126,7 +145,7 @@ describe("IconPicker", () => { it("should return to the first page when the search changes", async () => { const user = userEvent.setup(); - render(); + renderPicker(); await user.click(screen.getByRole("button", { name: /browse/i })); await user.click(screen.getByRole("button", { name: "Next page" })); @@ -139,7 +158,7 @@ describe("IconPicker", () => { it("should clear the search when the popover is reopened", async () => { const user = userEvent.setup(); - render(); + renderPicker(); await user.click(screen.getByRole("button", { name: /browse/i })); await user.type(screen.getByPlaceholderText("Search icons..."), "airplay"); @@ -159,7 +178,7 @@ describe("IconPicker", () => { it("should always open on the first page, even with a selection on a later page", async () => { const user = userEvent.setup(); // "zap" sorts near the end of the alphabetised list, well past page 1. - render(); + renderPicker({ currentIconUrl: "lucide:zap" }); await user.click(screen.getByRole("button", { name: /browse/i })); @@ -168,7 +187,7 @@ describe("IconPicker", () => { it("should reopen on the same page after closing with Escape", async () => { const user = userEvent.setup(); - render(); + renderPicker(); await user.click(screen.getByRole("button", { name: /browse/i })); await user.click(screen.getByRole("button", { name: "Next page" })); @@ -189,14 +208,14 @@ describe("IconPicker", () => { const user = userEvent.setup(); // Remounting mirrors the node style dialog closing and reopening, which // unmounts the picker and discards its page state. - const { unmount } = render(); + const { unmount } = renderPicker(); await user.click(screen.getByRole("button", { name: /browse/i })); await user.click(screen.getByRole("button", { name: "Next page" })); expect(screen.getByText(/Page 2 of/)).toBeInTheDocument(); unmount(); - render(); + renderPicker(); await user.click(screen.getByRole("button", { name: /browse/i })); expect(screen.getByText(/Page 1 of/)).toBeInTheDocument(); @@ -204,7 +223,7 @@ describe("IconPicker", () => { it("should reopen on the same page after selecting an icon", async () => { const user = userEvent.setup(); - render(); + renderPicker(); await user.click(screen.getByRole("button", { name: /browse/i })); await user.click(screen.getByRole("button", { name: "Next page" })); @@ -223,7 +242,7 @@ describe("IconPicker", () => { it("should show no results message for invalid search", async () => { const user = userEvent.setup(); - render(); + renderPicker(); await user.click(screen.getByRole("button", { name: /browse/i })); const searchInput = screen.getByPlaceholderText("Search icons..."); @@ -236,7 +255,7 @@ describe("IconPicker", () => { it("should call onSelect with lucide: reference when icon is clicked", async () => { const user = userEvent.setup(); const onSelect = vi.fn(); - render(); + renderPicker({ onSelect }); await user.click(screen.getByRole("button", { name: /browse/i })); @@ -245,38 +264,27 @@ describe("IconPicker", () => { }); const firstIcon = firstVisibleIcon(); - const iconName = firstIcon.getAttribute("title"); + const name = iconName(firstIcon); await user.click(firstIcon); - expect(onSelect).toHaveBeenCalledWith( - `lucide:${iconName}`, - "image/svg+xml", - ); + expect(onSelect).toHaveBeenCalledWith(`lucide:${name}`, "image/svg+xml"); }); it("should highlight the icon matching currentIconUrl", async () => { const user = userEvent.setup(); - render(); + renderPicker({ currentIconUrl: "lucide:airplay" }); await user.click(screen.getByRole("button", { name: /browse/i })); await waitFor(() => { - const airplayBtn = screen - .getAllByRole("button") - .find(btn => btn.title === "airplay"); - expect(airplayBtn).toBeDefined(); + const airplayBtn = screen.getByRole("button", { name: "airplay" }); expect(airplayBtn).toHaveAttribute("aria-pressed", "true"); }); }); it("should not highlight any icon when currentIconUrl is not a lucide ref", async () => { const user = userEvent.setup(); - render( - , - ); + renderPicker({ currentIconUrl: "data:image/svg+xml;base64,XXXX" }); await user.click(screen.getByRole("button", { name: /browse/i })); @@ -291,7 +299,7 @@ describe("IconPicker", () => { it("should close popover after selecting an icon", async () => { const user = userEvent.setup(); - render(); + renderPicker(); await user.click(screen.getByRole("button", { name: /browse/i })); diff --git a/packages/graph-explorer/src/components/IconPicker.tsx b/packages/graph-explorer/src/components/IconPicker.tsx index 0517b6556..d8ec32cc6 100644 --- a/packages/graph-explorer/src/components/IconPicker.tsx +++ b/packages/graph-explorer/src/components/IconPicker.tsx @@ -166,7 +166,7 @@ function IconButton({ }) { return (