Skip to content

fix(search): sort button layout + section dividers in filter panel (#981) #985

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.

Sign up for GitHub

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

Merged
ericsocrat merged 1 commit into from
Mar 19, 2026
Merged

fix(search): sort button layout + section dividers in filter panel (#981) #985

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
23 changes: 23 additions & 0 deletions frontend/src/components/search/FilterPanel.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -478,4 +478,27 @@ describe("FilterPanel", () => {
expect(screen.getAllByText("Clear all").length).toBeGreaterThanOrEqual(1);
});
});

// ─── Section Dividers ─────────────────────────────────────────────────────

it("renders section dividers between filter groups", async () => {
renderPanel();
await waitFor(() => {
expect(screen.getAllByText("Relevance").length).toBeGreaterThanOrEqual(1);
});
const separators = document.querySelectorAll("hr");
Comment on lines +485 to +489
Copy link

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

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

This test queries document.querySelectorAll("hr"), which isn't scoped to the rendered FilterPanel and can be influenced by other DOM content. Since renderPanel() returns the render result, prefer scoping to its container (or using Testing Library queries) to make the test deterministic; also consider asserting the expected divider count per rendered panel, since desktop+mobile may render duplicates.

Suggested change
renderPanel();
await waitFor(() => {
expect(screen.getAllByText("Relevance").length).toBeGreaterThanOrEqual(1);
});
const separators = document.querySelectorAll("hr");
const { container } = renderPanel();
await waitFor(() => {
expect(screen.getAllByText("Relevance").length).toBeGreaterThanOrEqual(1);
});
const separators = container.querySelectorAll("hr");

Copilot uses AI. Check for mistakes.
expect(separators.length).toBeGreaterThanOrEqual(5);
});

// ─── Sort Layout (col-span-2 on last odd button) ─────────────────────────

it("adds col-span-2 to last sort button when odd count", async () => {
renderPanel();
await waitFor(() => {
expect(screen.getAllByText("Calories").length).toBeGreaterThanOrEqual(1);
});
// "Calories" is the 5th (odd-last) sort button
const caloriesBtn = screen.getAllByText("Calories")[0].closest("button");
expect(caloriesBtn?.className).toContain("col-span-2");
Comment on lines +501 to +502
Copy link

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

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

caloriesBtn?.className can become undefined if the closest button isn't found, leading to a less clear failure mode. Since the presence of the element is required for the assertion, explicitly assert that caloriesBtn is non-null (or use a role-based query that returns the button) before checking for col-span-2.

Suggested change
const caloriesBtn = screen.getAllByText("Calories")[0].closest("button");
expect(caloriesBtn?.className).toContain("col-span-2");
const caloriesBtn = screen.getByRole("button", { name: /Calories/i });
expect(caloriesBtn.className).toContain("col-span-2");

Copilot uses AI. Check for mistakes.
});
});
15 changes: 14 additions & 1 deletion frontend/src/components/search/FilterPanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,14 +131,17 @@ export function FilterPanel({
label: t("filters.nutriScore"),
},
{ value: "calories" as const, label: t("filters.calories") },
].map((opt) => {
].map((opt, idx, arr) => {
const isActive = (filters.sort_by ?? "relevance") === opt.value;
const isLastOdd = arr.length % 2 === 1 && idx === arr.length - 1;
return (
<button
key={opt.value}
type="button"
onClick={() => setSortBy(opt.value)}
className={`rounded-lg px-3 py-2 text-xs font-medium transition-colors ${
isLastOdd ? "col-span-2" : ""
} ${
isActive
? "bg-brand-subtle text-brand ring-2 ring-brand/30"
: "bg-surface-muted text-foreground-secondary hover:bg-surface-subtle"
Expand Down Expand Up @@ -180,6 +183,8 @@ export function FilterPanel({
)}
</div>

<hr className="border-t border-border/40" />

Comment on lines +186 to +187
Copy link

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

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

The new

dividers are always rendered even when the adjacent section is conditionally absent (e.g., no categories/nutri_scores/etc.). This can result in dividers not actually separating two rendered groups (including potentially multiple consecutive dividers). Consider only rendering a divider when both the previous and next sections are present, or build a list of rendered sections and interleave separators between them.

Copilot uses AI. Check for mistakes.
{/* Categories */}
{data && (data.categories?.length ?? 0) > 0 && (
<div>
Comment on lines +186 to 190
Copy link

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

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

These

elements are inserted as additional direct children inside a space-y-5 container, which will add vertical spacing both above and below each divider (effectively doubling the gap between sections vs. before). If the intent is to keep the same spacing and just add a subtle separator, consider switching to a divide-y approach on a wrapper that only contains the filter groups (excluding the Clear button), or remove space-y-5 and explicitly control margins/padding around the divider.

Suggested change
<hr className="border-t border-border/40" />
{/* Categories */}
{data && (data.categories?.length ?? 0) > 0 && (
<div>
{/* Categories */}
{data && (data.categories?.length ?? 0) > 0 && (
<div className="border-t border-border/40 pt-5">

Copilot uses AI. Check for mistakes.
Expand Down Expand Up @@ -218,6 +223,8 @@ export function FilterPanel({
</div>
)}

<hr className="border-t border-border/40" />

{/* Nutri-Score */}
{data && (data.nutri_scores?.length ?? 0) > 0 && (
<div>
Expand Down Expand Up @@ -253,6 +260,8 @@ export function FilterPanel({
</div>
)}

<hr className="border-t border-border/40" />

{/* NOVA Group */}
{data && (data.nova_groups?.length ?? 0) > 0 && (
<div>
Expand Down Expand Up @@ -287,6 +296,8 @@ export function FilterPanel({
</div>
)}

<hr className="border-t border-border/40" />

{/* Allergen-Free */}
{data && (data.allergens?.length ?? 0) > 0 && (
<div>
Expand Down Expand Up @@ -325,6 +336,8 @@ export function FilterPanel({
</div>
)}

<hr className="border-t border-border/40" />

{/* Min TryVit Score Slider */}
<div>
<h3 className="mb-2 text-xs font-semibold uppercase tracking-wider text-foreground-secondary">
Expand Down
Loading