diff --git a/src/components/layout/HeaderProfileMenu.tsx b/src/components/layout/HeaderProfileMenu.tsx index b266fb4..85a32b4 100644 --- a/src/components/layout/HeaderProfileMenu.tsx +++ b/src/components/layout/HeaderProfileMenu.tsx @@ -87,7 +87,7 @@ export function HeaderProfileMenu() { diff --git a/src/components/server-catalog/CatalogApiKeyDialog.tsx b/src/components/server-catalog/CatalogApiKeyDialog.tsx index 9a2322e..6f9521c 100644 --- a/src/components/server-catalog/CatalogApiKeyDialog.tsx +++ b/src/components/server-catalog/CatalogApiKeyDialog.tsx @@ -180,7 +180,6 @@ export function CatalogApiKeyDialog({ }} disabled={isSubmitting} > - {/* SelectTrigger is w-fit by default; full width lines it up with the inputs above. */} { + it("renders label linked to control via htmlFor", () => { + render( + + {(controlProps) => } + , + ); + const label = screen.getByText("Name"); + expect(label).toHaveAttribute("for", "name"); + expect(screen.getByRole("textbox")).toHaveAttribute("id", "name"); + }); + + it("renders error message with matching id", () => { + render( + + {(controlProps) => } + , + ); + const error = screen.getByRole("alert"); + expect(error).toHaveTextContent("Required"); + expect(error).toHaveAttribute("id", "email-error"); + }); + + it("injects aria-invalid=true on the control when error is set", () => { + render( + + {(controlProps) => } + , + ); + expect(screen.getByRole("textbox")).toHaveAttribute("aria-invalid", "true"); + }); + + it("injects aria-describedby pointing to error id on the control", () => { + render( + + {(controlProps) => } + , + ); + expect(screen.getByRole("textbox")).toHaveAttribute("aria-describedby", "email-error"); + }); + + it('treats error="" as invalid (not the same as no error): wires aria-invalid/describedby and renders the alert', () => { + render( + + {(controlProps) => } + , + ); + expect(screen.getByRole("textbox")).toHaveAttribute("aria-invalid", "true"); + expect(screen.getByRole("textbox")).toHaveAttribute("aria-describedby", "email-error"); + expect(screen.getByRole("alert")).toHaveAttribute("id", "email-error"); + }); + + it("does not render error block when no error", () => { + render( + + {(controlProps) => } + , + ); + expect(screen.queryByRole("alert")).not.toBeInTheDocument(); + }); + + it("renders hint text with an id, wired into aria-describedby, when no error", () => { + render( + + {(controlProps) => } + , + ); + const hint = screen.getByText("Help text"); + expect(hint).toHaveAttribute("id", "name-hint"); + expect(screen.getByRole("textbox")).toHaveAttribute("aria-describedby", "name-hint"); + }); + + it("prefers the error over the hint: hides hint and points describedby at the error", () => { + render( + + {(controlProps) => } + , + ); + expect(screen.queryByText("Help text")).not.toBeInTheDocument(); + expect(screen.getByRole("textbox")).toHaveAttribute("aria-describedby", "name-error"); + }); + + it("does not let labelProps override which control the label targets", () => { + render( + + {(controlProps) => } + , + ); + expect(screen.getByText("Email")).toHaveAttribute("for", "email"); + }); + + it("supports a render-prop child for composite controls, applying props to the caller-chosen element", () => { + // A Select-shaped composite: the top-level element (the "Select" stand-in) + // renders no DOM node of its own, and forwards nothing to its child. The + // render-prop form lets the caller put the control props on the actual + // DOM-facing element instead. + function FakeSelectRoot({ children }: { children: ReactNode }) { + return
{children}
; + } + + render( + + {(controlProps) => ( + + + + )} + , + ); + + const trigger = screen.getByTestId("select-trigger"); + expect(trigger).toHaveAttribute("id", "visibility"); + expect(trigger).toHaveAttribute("aria-invalid", "true"); + expect(trigger).toHaveAttribute("aria-describedby", "visibility-error"); + // The wrapper the caller chose not to tag stays untouched. + expect(screen.getByTestId("select-root")).not.toHaveAttribute("aria-invalid"); + }); +}); diff --git a/src/components/ui/field.tsx b/src/components/ui/field.tsx new file mode 100644 index 0000000..79e4418 --- /dev/null +++ b/src/components/ui/field.tsx @@ -0,0 +1,66 @@ +import * as React from "react"; +import { Label } from "@/components/ui/label"; +import { cn } from "@/lib/utils"; + +/** Props Field computes for its control: id, invalid state, describedby chain. */ +export interface FieldControlProps { + id: string; + "aria-invalid"?: boolean; + "aria-describedby"?: string; +} + +interface FieldProps { + /** `id` that links the label to its control via `htmlFor`. */ + id: string; + label: React.ReactNode; + /** Optional help text shown below the control. */ + hint?: React.ReactNode; + /** Validation message; when present the error colour applies. */ + error?: string; + /** Render prop for the control; receives `id`/`aria-invalid`/`aria-describedby` to spread onto whichever element is DOM-facing (e.g. `SelectTrigger` inside `Select`). */ + children: (controlProps: FieldControlProps) => React.ReactNode; + className?: string; + /** Additional props forwarded to the label. `htmlFor` always comes from `id`. */ + labelProps?: Omit, "htmlFor">; +} + +/** + * Standardised field layout: label -> control -> hint/error. + * Defines the label-to-control gap once so forms don't diverge. + */ +function Field({ id, label, hint, error, children, className, labelProps }: FieldProps) { + // Presence check, not truthiness: an explicit error="" still means invalid + // (e.g. a message that hasn't resolved yet), so it must not be treated the + // same as "no error" and silently drop aria-invalid/aria-describedby. + const hasError = error !== undefined; + const errorId = hasError ? `${id}-error` : undefined; + const hintId = hint && !hasError ? `${id}-hint` : undefined; + + const controlProps: FieldControlProps = { + id, + "aria-invalid": hasError ? true : undefined, + "aria-describedby": errorId ?? hintId, + }; + + return ( +
+ {/* labelProps spreads before htmlFor so it can never override which control this labels. */} + + {children(controlProps)} + {hintId && ( +

+ {hint} +

+ )} + {hasError && ( + + )} +
+ ); +} + +export { Field }; diff --git a/src/components/ui/label.test.tsx b/src/components/ui/label.test.tsx index 996aa53..9de667c 100644 --- a/src/components/ui/label.test.tsx +++ b/src/components/ui/label.test.tsx @@ -1,7 +1,15 @@ -import { describe, it, expect, beforeEach } from "vitest"; +import { describe, it, expect, beforeEach, afterEach } from "vitest"; import { render } from "@testing-library/react"; import React from "react"; +import fs from "node:fs"; +import path from "node:path"; import { Label } from "./label"; +import { cn } from "@/lib/utils"; + +// The CVA base string itself, mirroring label.tsx: leading-none is no longer +// in it, so a call-site "text-sm" can never strip it via tailwind-merge. +const labelBaseClasses = + "text-sm font-medium peer-disabled:cursor-not-allowed peer-disabled:opacity-70"; describe("Label", () => { beforeEach(() => { @@ -57,7 +65,13 @@ describe("Label", () => { expect(el).toHaveClass("text-sm"); expect(el).toHaveClass("font-medium"); - expect(el).toHaveClass("leading-none"); + }); + + it("should carry data-slot=label so globals.css can set its line-height", () => { + const { container } = render(); + const el = container.querySelector("label"); + + expect(el).toHaveAttribute("data-slot", "label"); }); it("should have peer-disabled styling classes", () => { @@ -77,7 +91,13 @@ describe("Label", () => { expect(el).toHaveClass("custom-label"); expect(el).toHaveClass("text-sm"); expect(el).toHaveClass("font-medium"); - expect(el).toHaveClass("leading-none"); + }); + + it("preserves the label's line-height when text-sm is re-passed as className", () => { + // text-sm can no longer collide with leading-none in twMerge, since + // leading-none isn't in the CVA string at all anymore (it's CSS-only). + const merged = cn(labelBaseClasses, "text-sm"); + expect(merged).not.toMatch(/\bleading-(?!none)\w+/); }); it("should merge multiple custom classes with default classes", () => { @@ -488,7 +508,6 @@ describe("Label", () => { expect(el).toHaveClass("text-sm"); expect(el).toHaveClass("font-medium"); - expect(el).toHaveClass("leading-none"); expect(el).toHaveClass("custom"); }); @@ -545,7 +564,6 @@ describe("Label", () => { const el = container.querySelector("label"); expect(el).toHaveClass("text-sm"); expect(el).toHaveClass("font-medium"); - expect(el).toHaveClass("leading-none"); expect(el).toHaveClass("custom"); }); @@ -573,4 +591,52 @@ describe("Label", () => { expect(el).toHaveAttribute("for", "test"); }); }); + + // Regression test for the cascade-layer bug: [data-slot="label"] lives + // unlayered in index.css so it can outrank Tailwind's `.text-sm` (which is + // in `@layer utilities`). Being unlayered means it would otherwise beat + // *any* layered rule unconditionally, regardless of specificity — + // permanently blocking a caller's own "leading-*" override. The + // :not([class*="leading-"]) guard is what makes that override possible + // again; this fails if the guard is ever dropped. + describe('[data-slot="label"] line-height CSS rule', () => { + const css = fs.readFileSync(path.resolve(__dirname, "../../index.css"), "utf-8"); + const rule = css.match(/\[data-slot="label"\][^{]*\{[^}]*\}/); + + it("exists and is guarded against callers that already set a leading-* class", () => { + expect(rule).not.toBeNull(); + expect(rule![0]).toMatch(/:not\(\[class\*="leading-"\]\)/); + }); + + describe("applied behaviour", () => { + const injected = document.createElement("style"); + + afterEach(() => { + injected.remove(); + document.body.innerHTML = ""; + }); + + function renderLabel(className: string) { + injected.textContent = rule![0]; + document.head.appendChild(injected); + const el = document.createElement("label"); + el.setAttribute("data-slot", "label"); + el.className = className; + document.body.appendChild(el); + return el; + } + + it("forces line-height:1 on a label with no leading-* class", () => { + const el = renderLabel("text-sm font-medium"); + expect(getComputedStyle(el).lineHeight).toBe("1"); + }); + + it("does not clobber a label that carries an explicit leading-* class (e.g. ExposeComponentsForm's OAuth description)", () => { + const el = renderLabel( + "text-sm font-normal leading-relaxed text-neutral-600 dark:text-neutral-400 cursor-pointer", + ); + expect(getComputedStyle(el).lineHeight).not.toBe("1"); + }); + }); + }); }); diff --git a/src/components/ui/label.tsx b/src/components/ui/label.tsx index 2f03e84..d2b1de7 100644 --- a/src/components/ui/label.tsx +++ b/src/components/ui/label.tsx @@ -4,15 +4,23 @@ import { cva, type VariantProps } from "class-variance-authority"; import { cn } from "@/lib/utils"; +// leading-none lives in globals.css via [data-slot="label"] instead of here: +// twMerge treats line-height utilities as conflicting with font-size, so a +// call-site "text-sm" would silently strip a "leading-none" in this string. const labelVariants = cva( - "text-sm font-medium leading-none peer-disabled:cursor-not-allowed peer-disabled:opacity-70", + "text-sm font-medium peer-disabled:cursor-not-allowed peer-disabled:opacity-70", ); const Label = React.forwardRef< React.ElementRef, React.ComponentPropsWithoutRef & VariantProps >(({ className, ...props }, ref) => ( - + )); Label.displayName = "Label"; diff --git a/src/components/ui/select.test.tsx b/src/components/ui/select.test.tsx index 43e1e41..8409ac0 100644 --- a/src/components/ui/select.test.tsx +++ b/src/components/ui/select.test.tsx @@ -28,6 +28,30 @@ describe("Select Components", () => { expect(screen.getByTestId("trigger")).toBeInTheDocument(); }); + it("allows className h-10 to override the default h-9 (CVA variant, not data-[size=*])", () => { + render( + , + ); + const trigger = screen.getByTestId("trigger-h10"); + expect(trigger.className).not.toContain("h-9"); + expect(trigger.className).toContain("h-10"); + }); + + it("defaults SelectTrigger to w-full", () => { + render( + , + ); + expect(screen.getByTestId("trigger-width")).toHaveClass("w-full"); + }); + it("renders SelectTrigger with sm size", () => { render(