Critique FormGroup, Tabs and ConditionalField.

Author: Pigna

src/components/Form/ConditionalField/ConditionalField.module.scss

@@ -1,7 +1,51 @@
+@use '../../../styles/mixins' as *;
+
+/* ---- keepMounted path: grid-template-rows collapse + opacity fade ---- */
+
 .keepMounted {
-  /* Wrapper used only when keepMounted=true */
+  display: grid;
+  grid-template-rows: 1fr;
+  opacity: 1;
+  transition:
+    grid-template-rows var(--transition-normal),
+    opacity var(--transition-fast);
+
+  @include reduced-motion {
+    transition: none;
+  }
 }
 
 .hidden {
-  display: none;
+  grid-template-rows: 0fr;
+  opacity: 0;
+  pointer-events: none;
 }
+
+/* Inner wrapper required for the grid collapse technique */
+.inner {
+  overflow: hidden;
+  min-height: 0;
+}
+
+/* ---- Default animated path: fade-in entrance on mount ---- */
+
+.enter {
+  animation: cfFadeIn var(--transition-normal) ease-out both;
+
+  @include reduced-motion {
+    animation: none;
+  }
+}
+
+@keyframes cfFadeIn {
+  from {
+    opacity: 0;
+    transform: translateY(-0.25rem);
+  }
+
+  to {
+    opacity: 1;
+    transform: translateY(0);
+  }
+}
+

src/components/Form/ConditionalField/ConditionalField.stories.tsx

@@ -64,7 +64,7 @@ export const SelectDriven: Story = {
 };
 
 /**
- * With `keepMounted`, the hidden content stays in the DOM (aria-hidden).
+ * With `keepMounted`, the hidden content stays in the DOM and **animates in/out** smoothly.
  * Useful when you need to preserve controlled state across visibility toggles.
  */
 export const KeepMounted: Story = {

src/components/Form/ConditionalField/ConditionalField.test.tsx

@@ -30,22 +30,23 @@ describe('ConditionalField', () => {
     expect(screen.getByTestId('content')).toBeInTheDocument();
   });
 
-  it('sets aria-hidden on wrapper when keepMounted and hidden', () => {
+  it('sets inert on wrapper when keepMounted and hidden', () => {
     const { container } = render(
       <ConditionalField show={false} keepMounted>
         <span>inner</span>
       </ConditionalField>,
     );
-    expect(container.firstChild).toHaveAttribute('aria-hidden', 'true');
+    // inert attribute is present (implicitly sets aria-hidden per HTML spec)
+    expect(container.firstChild).toHaveAttribute('inert');
   });
 
-  it('does not set aria-hidden when keepMounted and visible', () => {
+  it('does not set inert when keepMounted and visible', () => {
     const { container } = render(
       <ConditionalField show keepMounted>
         <span>inner</span>
       </ConditionalField>,
     );
-    expect(container.firstChild).not.toHaveAttribute('aria-hidden');
+    expect(container.firstChild).not.toHaveAttribute('inert');
   });
 
   it('renders nothing (null) when show is false without keepMounted', () => {
@@ -56,4 +57,24 @@ describe('ConditionalField', () => {
     );
     expect(container.firstChild).toBeNull();
   });
+
+  it('wraps children in a div when animated is true', () => {
+    const { container } = render(
+      <ConditionalField show animated>
+        <span data-testid="content">hello</span>
+      </ConditionalField>,
+    );
+    expect(container.firstChild?.nodeName).toBe('DIV');
+    expect(screen.getByTestId('content')).toBeInTheDocument();
+  });
+
+  it('does not render wrapper div when not animated (default)', () => {
+    const { container } = render(
+      <ConditionalField show>
+        <span data-testid="content">hello</span>
+      </ConditionalField>,
+    );
+    // Fragment renders — firstChild is the span itself
+    expect(container.firstChild?.nodeName).toBe('SPAN');
+  });
 });

src/components/Form/ConditionalField/ConditionalField.tsx

@@ -5,12 +5,18 @@ export interface ConditionalFieldProps {
   /** When true, children are shown. When false, they are hidden (or unmounted). */
   show: boolean;
   /**
-   * When true, children remain in the DOM but are visually hidden when `show` is false.
+   * When true, children remain in the DOM and animate in/out instead of mounting/unmounting.
    * Useful for preserving controlled input state across visibility toggles.
    * @default false
    */
   keepMounted?: boolean;
-  /** Additional class name for the wrapper element (only rendered when keepMounted). */
+  /**
+   * When true (and keepMounted is false), wraps children in a div with a fade-in entrance
+   * animation on mount. Adds one wrapper element to the DOM.
+   * @default false
+   */
+  animated?: boolean;
+  /** Additional class name for the wrapper element (only rendered when keepMounted or animated). */
   className?: string;
   children: ReactNode;
 }
@@ -22,16 +28,19 @@ export interface ConditionalFieldProps {
  * accessible default (hidden content is not in the DOM or focus order).
  *
  * Set `keepMounted` to preserve DOM state (e.g. controlled inputs that must
- * retain their value while hidden).
+ * retain their value while hidden). Content animates in and out smoothly.
+ *
+ * Set `animated` (with default mode) to get a fade-in entrance animation on mount.
+ * Note: this wraps children in a `<div>` — be aware of layout implications.
  *
  * @example
  * ```tsx
- * // Unmounts when condition is false (default)
+ * // Default: unmounts when false
  * <ConditionalField show={country === 'us'}>
  *   <Input label="State" />
  * </ConditionalField>
  *
- * // Keeps mounted but visually hidden
+ * // Keeps mounted, animates in/out
  * <ConditionalField show={hasBillingAddress} keepMounted>
  *   <FormGroup title="Billing Address">...</FormGroup>
  * </ConditionalField>
@@ -40,19 +49,28 @@ export interface ConditionalFieldProps {
 export function ConditionalField({
   show,
   keepMounted = false,
+  animated = false,
   className,
   children,
 }: ConditionalFieldProps) {
   if (keepMounted) {
     const wrapperClasses = [styles.keepMounted, !show ? styles.hidden : '', className]
       .filter(Boolean)
       .join(' ');
+    // inert implicitly applies aria-hidden — no need to set both
     return (
-      <div className={wrapperClasses} aria-hidden={!show || undefined} inert={!show || undefined}>
-        {children}
+      <div className={wrapperClasses} inert={!show || undefined}>
+        <div className={styles.inner}>{children}</div>
       </div>
     );
   }
 
-  return show ? <>{children}</> : null;
+  if (!show) return null;
+
+  if (animated) {
+    return <div className={[styles.enter, className].filter(Boolean).join(' ')}>{children}</div>;
+  }
+
+  return <>{children}</>;
 }
+

src/components/Form/FormGroup/FormGroup.module.scss

@@ -5,6 +5,8 @@
   border-radius: var(--radius-lg);
   padding: var(--space-5) var(--space-6);
   margin: 0;
+  /* Prevent fieldset from overflowing flex/grid containers */
+  min-width: 0;
   font-family: var(--font-family);
 }
 
@@ -27,4 +29,6 @@
   display: flex;
   flex-direction: column;
   gap: var(--space-4);
+  /* Consistent gap between legend and first field regardless of description */
+  padding-top: var(--space-3);
 }

src/components/Tabs/Tabs.test.tsx

@@ -84,6 +84,17 @@ describe('Tabs', () => {
     expect(screen.getByText('Panel A')).toBeInTheDocument();
   });
 
+  it('arrow key navigation skips disabled tabs', async () => {
+    const user = userEvent.setup();
+    render(<SimpleTabs defaultTab="b" />);
+    const tabB = screen.getByRole('tab', { name: 'Tab B' });
+    tabB.focus();
+    // ArrowRight from B would normally go to C (disabled) — should skip to A (wraps)
+    await user.keyboard('{ArrowRight}');
+    expect(screen.getByText('Panel A')).toBeInTheDocument();
+    expect(screen.queryByText('Panel C')).not.toBeInTheDocument();
+  });
+
   it('throws when Tab is used outside Tabs', () => {
     expect(() =>
       render(<Tab id="x">orphan</Tab>),

src/components/Tabs/Tabs.tsx

@@ -6,6 +6,7 @@ import {
   useState,
   type HTMLAttributes,
   type KeyboardEvent,
+  type RefObject,
   type ReactNode,
   type Ref,
 } from 'react';
@@ -17,7 +18,10 @@ interface TabsContextValue {
   activeTab: string;
   setActiveTab: (id: string) => void;
   registerTab: (id: string) => void;
-  tabIds: React.MutableRefObject<string[]>;
+  tabIds: RefObject<string[]>;
+  /** Stable prefix generated once per Tabs instance — shared by Tab and TabPanel
+   *  so aria-controls / aria-labelledby IDs always match. */
+  instanceId: string;
 }
 
 const TabsContext = createContext<TabsContextValue | null>(null);
@@ -70,6 +74,7 @@ export function Tabs({
   ref,
   ...rest
 }: TabsProps) {
+  const instanceId = useId();
   const tabIds = useRef<string[]>([]);
   const [internalTab, setInternalTab] = useState(defaultTab ?? '');
 
@@ -94,7 +99,7 @@ export function Tabs({
   const classNames = [styles.tabs, className].filter(Boolean).join(' ');
 
   return (
-    <TabsContext value={{ activeTab, setActiveTab, registerTab, tabIds }}>
+    <TabsContext value={{ activeTab, setActiveTab, registerTab, tabIds, instanceId }}>
       <div ref={ref} className={classNames} {...rest}>
         {children}
       </div>
@@ -148,40 +153,55 @@ export interface TabProps extends Omit<HTMLAttributes<HTMLButtonElement>, 'id'>
  * An individual tab button. Must be a direct child of `<TabList>`.
  *
  * Keyboard navigation:
- * - `ArrowLeft` / `ArrowRight` — move focus between tabs
- * - `Home` — focus first tab
- * - `End` — focus last tab
- * - `Enter` / `Space` — activate focused tab
+ * - `ArrowLeft` / `ArrowRight` — move focus between tabs (skips disabled tabs)
+ * - `Home` — focus first enabled tab
+ * - `End` — focus last enabled tab
+ * - `Enter` / `Space` — activate focused tab (native button behaviour)
  */
 export function Tab({ id, disabled = false, children, className, ref, ...rest }: TabProps) {
-  const { activeTab, setActiveTab, registerTab, tabIds } = useTabsContext();
-  const generatedId = useId();
-  const buttonId = `tab-${id}-${generatedId}`;
-  const panelId = `tabpanel-${id}-${generatedId.replace(/:/g, '')}`;
+  const { activeTab, setActiveTab, registerTab, tabIds, instanceId } = useTabsContext();
+
+  // IDs derived from the shared instanceId so Tab and TabPanel always agree
+  const buttonId = `tab-${instanceId}-${id}`;
+  const panelId = `tabpanel-${instanceId}-${id}`;
 
-  // Register on first render
   registerTab(id);
 
   const isActive = activeTab === id;
   const classNames = [styles.tab, isActive ? styles.active : '', className].filter(Boolean).join(' ');
 
+  const isTabDisabled = (tabId: string) => {
+    const btn = document.querySelector<HTMLButtonElement>(`[data-tab-id="${tabId}"]`);
+    return btn?.disabled ?? false;
+  };
+
   const handleKeyDown = (e: KeyboardEvent<HTMLButtonElement>) => {
     const ids = tabIds.current;
     const currentIndex = ids.indexOf(id);
 
+    const findNext = (startIdx: number, direction: 1 | -1): number | null => {
+      for (let i = 1; i < ids.length; i++) {
+        const idx = (startIdx + direction * i + ids.length) % ids.length;
+        if (!isTabDisabled(ids[idx])) return idx;
+      }
+      return null;
+    };
+
     let nextIndex: number | null = null;
-    if (e.key === 'ArrowRight') nextIndex = (currentIndex + 1) % ids.length;
-    else if (e.key === 'ArrowLeft') nextIndex = (currentIndex - 1 + ids.length) % ids.length;
-    else if (e.key === 'Home') nextIndex = 0;
-    else if (e.key === 'End') nextIndex = ids.length - 1;
+    if (e.key === 'ArrowRight') nextIndex = findNext(currentIndex, 1);
+    else if (e.key === 'ArrowLeft') nextIndex = findNext(currentIndex, -1);
+    else if (e.key === 'Home') nextIndex = ids.findIndex((tid) => !isTabDisabled(tid));
+    else if (e.key === 'End') {
+      for (let i = ids.length - 1; i >= 0; i--) {
+        if (!isTabDisabled(ids[i])) { nextIndex = i; break; }
+      }
+    }
 
-    if (nextIndex !== null) {
+    if (nextIndex !== null && nextIndex >= 0) {
       e.preventDefault();
       const nextId = ids[nextIndex];
       setActiveTab(nextId);
-      // Move DOM focus to the next tab button
-      const nextButton = document.querySelector<HTMLButtonElement>(`[data-tab-id="${nextId}"]`);
-      nextButton?.focus();
+      document.querySelector<HTMLButtonElement>(`[data-tab-id="${nextId}"]`)?.focus();
     }
 
     rest.onKeyDown?.(e);
@@ -228,10 +248,11 @@ export interface TabPanelProps extends HTMLAttributes<HTMLDivElement> {
  * ```
  */
 export function TabPanel({ id, children, className, ref, ...rest }: TabPanelProps) {
-  const { activeTab } = useTabsContext();
-  const generatedId = useId();
-  const panelId = `tabpanel-${id}-${generatedId.replace(/:/g, '')}`;
-  const buttonId = `tab-${id}-${generatedId}`;
+  const { activeTab, instanceId } = useTabsContext();
+
+  // IDs derived from the shared instanceId — must mirror what Tab produces
+  const panelId = `tabpanel-${instanceId}-${id}`;
+  const buttonId = `tab-${instanceId}-${id}`;
 
   if (activeTab !== id) return null;
 
@@ -251,3 +272,4 @@ export function TabPanel({ id, children, className, ref, ...rest }: TabPanelProp
     </div>
   );
 }
+