From f4e4d9357e9677c137de492294819ce5897e1e22 Mon Sep 17 00:00:00 2001 From: Vivek Date: Tue, 28 Jul 2026 01:48:18 +0530 Subject: [PATCH 1/2] Revert "fix(ui): make the accordion trigger hover a state layer (#1137)" This reverts commit 30b94783886204e0e4b444cd05e59d5231fd4f4a. --- .../packages/registry/components/accordion.ts | 39 +++++++------------ 1 file changed, 14 insertions(+), 25 deletions(-) diff --git a/packages/ui/packages/registry/components/accordion.ts b/packages/ui/packages/registry/components/accordion.ts index a134f9117..04bc744f4 100644 --- a/packages/ui/packages/registry/components/accordion.ts +++ b/packages/ui/packages/registry/components/accordion.ts @@ -22,7 +22,7 @@ * Chrome 120+, Safari 17.2+, Firefox 130+. Migrated from the prior * custom element set. * - * Design tokens used: --border, --ring, --accent, --accent-foreground. + * Design tokens used: --border, --ring, --foreground. * * @example * ```html @@ -67,31 +67,20 @@ export const accordionItemClass = (): string => 'group border-b last:border-b-0' * prop combines both; native HTML has no `disabled` on
. */ export const accordionTriggerClass = (opts: { disabled?: boolean } = {}): string => { - // Hover is a STATE LAYER (the ghost-button / dropdown-item pair), not an - // underline and not a text dim. An underline reads as a link affordance, - // and this is a disclosure control on a , not a navigation. A dim - // was tried and rejected: reduced prominence is this kit's DISABLED - // vocabulary (opacity-50, including on this very trigger), so dimming on - // hover said "unavailable" and "under your pointer" on the same axis, and - // it lowered text contrast at the exact moment of interaction. The tint - // also reveals the full-width hit area, which a text change cannot. - // (shadcn's React accordion underlines here; this is a deliberate - // divergence in presentation only, so the API parity that matters, variant - // and size names and the data attributes, is untouched.) + // Hover dims the label instead of underlining it. An underline reads as a + // link affordance, and this is a disclosure control on a , not a + // navigation. A slight drop in text weight reads as "this responds to you" + // without borrowing another control's vocabulary, and it pairs with the + // chevron that rotates on open. (shadcn's React accordion underlines here; + // this is a deliberate divergence in presentation only, so the API parity + // that matters, variant and size names and the data attributes, is + // untouched.) // - // The -mx-2/px-2 pair keeps the label's left edge aligned with the panel - // content below (which has no horizontal padding either) while giving the - // tint breathing room; the tint intentionally extends 8px past the item - // dividers on BOTH sides, the same look as the webjs.dev docs sidebar - // links. There is deliberately no w-full here: a summary styled as flex is - // a block-level box that already fills its containing block, and width:100% - // plus symmetric negative margins is over-constrained, so the browser drops - // the end-side margin and the tint bleeds left while stopping short on the - // right (mirrored in RTL). rounded-md keeps the tint from reading as a - // full-bleed table row, and shapes the focus ring as a side effect. The - // label inherits its rest colour, so a wrapper that sets its own colour - // keeps it until hover. - const base = 'flex cursor-pointer list-none items-center justify-between gap-4 rounded-md -mx-2 px-2 py-4 text-left text-sm font-medium outline-none transition-all hover:bg-accent hover:text-accent-foreground focus-visible:ring-2 focus-visible:ring-ring/50 marker:hidden [&::-webkit-details-marker]:hidden'; + // The label carries `text-foreground` explicitly rather than inheriting it, + // because the hover target has to be a known starting colour: inheriting an + // author's own colour and then dimming to 80% of the THEME foreground would + // shift the hue, not just the weight. + const base = 'flex w-full cursor-pointer list-none items-center justify-between gap-4 py-4 text-left text-sm font-medium text-foreground outline-none transition-all hover:text-foreground/80 focus-visible:ring-2 focus-visible:ring-ring/50 marker:hidden [&::-webkit-details-marker]:hidden'; if (opts.disabled) return `${base} pointer-events-none cursor-not-allowed opacity-50`; return base; }; From 54121235dcb78817a3ef5bb12bf9e6f6f4e5454d Mon Sep 17 00:00:00 2001 From: Vivek Date: Tue, 28 Jul 2026 01:51:49 +0530 Subject: [PATCH 2/2] revert: restore the accordion trigger's hover underline Completes the revert. The trigger is now byte-identical to what it was before either of the two hover changes, back on shadcn's underline. The reasoning I gave for replacing it (an underline reads as a link, and this is a disclosure control) was not wrong on its own terms, but it was not a problem anyone had, and shadcn parity is the stronger default for a kit whose whole promise is that shadcn knowledge transfers. --- .../ui/packages/registry/components/accordion.ts | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/packages/ui/packages/registry/components/accordion.ts b/packages/ui/packages/registry/components/accordion.ts index 04bc744f4..968bc1ea4 100644 --- a/packages/ui/packages/registry/components/accordion.ts +++ b/packages/ui/packages/registry/components/accordion.ts @@ -67,20 +67,7 @@ export const accordionItemClass = (): string => 'group border-b last:border-b-0' * prop combines both; native HTML has no `disabled` on
. */ export const accordionTriggerClass = (opts: { disabled?: boolean } = {}): string => { - // Hover dims the label instead of underlining it. An underline reads as a - // link affordance, and this is a disclosure control on a , not a - // navigation. A slight drop in text weight reads as "this responds to you" - // without borrowing another control's vocabulary, and it pairs with the - // chevron that rotates on open. (shadcn's React accordion underlines here; - // this is a deliberate divergence in presentation only, so the API parity - // that matters, variant and size names and the data attributes, is - // untouched.) - // - // The label carries `text-foreground` explicitly rather than inheriting it, - // because the hover target has to be a known starting colour: inheriting an - // author's own colour and then dimming to 80% of the THEME foreground would - // shift the hue, not just the weight. - const base = 'flex w-full cursor-pointer list-none items-center justify-between gap-4 py-4 text-left text-sm font-medium text-foreground outline-none transition-all hover:text-foreground/80 focus-visible:ring-2 focus-visible:ring-ring/50 marker:hidden [&::-webkit-details-marker]:hidden'; + const base = 'flex w-full cursor-pointer list-none items-center justify-between gap-4 py-4 text-left text-sm font-medium outline-none transition-all hover:underline focus-visible:ring-2 focus-visible:ring-ring/50 marker:hidden [&::-webkit-details-marker]:hidden'; if (opts.disabled) return `${base} pointer-events-none cursor-not-allowed opacity-50`; return base; };