replacing the fieldset/legend in RadioGroup with a simplier div/span

NYPL · Jan 7, 2025 · 1f28940 · 1f28940
1 parent 7092fa2
commit 1f28940
Show file tree

Hide file tree

Showing 8 changed files with 191 additions and 151 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -8,6 +8,10 @@ Currently, this repo is in Prerelease. When it is released, this project will ad
 
 ## Prerelease
 
+### Updates
+
+- Replaces the `fieldset` and `legend` wrappers in the `RadioGroup` with a div and span for the title. The main div wrapper around the `Radio` components already has a `role="radiogroup"` values which makes the `fieldset` redundant.
+
 ## 3.5.1 (December 19, 2024)
 
 ### Adds

diff --git a/src/components/Header/components/__snapshots__/HeaderSearchForm.test.tsx.snap b/src/components/Header/components/__snapshots__/HeaderSearchForm.test.tsx.snap
@@ -96,13 +96,15 @@ exports[`HeaderSearchForm renders the UI snapshot correctly 1`] = `
           className="css-7ajbp2"
           id="search-header-child1-grandchild0"
         >
-          <fieldset
-            className="css-anp9yh"
+          <div
+            className="css-1piljje"
             id="radio-group-search-type"
           >
-            <legend>
+            <span
+              className="css-0"
+            >
               Type of search
-            </legend>
+            </span>
             <div
               className="chakra-radio-group css-0"
               role="radiogroup"
@@ -291,7 +293,7 @@ exports[`HeaderSearchForm renders the UI snapshot correctly 1`] = `
                 className="css-0"
               />
             </div>
-          </fieldset>
+          </div>
         </div>
       </div>
     </div>
@@ -395,13 +397,15 @@ exports[`HeaderSearchForm renders the UI snapshot correctly 2`] = `
           className="css-7ajbp2"
           id="search-header-child1-grandchild0"
         >
-          <fieldset
-            className="css-anp9yh"
+          <div
+            className="css-1piljje"
             id="radio-group-search-type"
           >
-            <legend>
+            <span
+              className="css-0"
+            >
               Type of search
-            </legend>
+            </span>
             <div
               className="chakra-radio-group css-0"
               role="radiogroup"
@@ -590,7 +594,7 @@ exports[`HeaderSearchForm renders the UI snapshot correctly 2`] = `
                 className="css-0"
               />
             </div>
-          </fieldset>
+          </div>
         </div>
       </div>
     </div>

diff --git a/src/components/RadioGroup/RadioGroup.mdx b/src/components/RadioGroup/RadioGroup.mdx
@@ -9,10 +9,10 @@ import { changelogData } from "./radioGroupChangelogData";
 
 # RadioGroup
 
-| Component Version | DS Version |
-| ----------------- | ---------- |
-| Added             | `0.25.0`   |
-| Latest            | `3.3.2`    |
+| Component Version | DS Version   |
+| ----------------- | ------------ |
+| Added             | `0.25.0`     |
+| Latest            | `Prerelease` |
 
 ## Table of Contents
 
@@ -37,8 +37,7 @@ import { changelogData } from "./radioGroupChangelogData";
 
 ## Accessibility
 
-The `RadioGroup` renders a group of `Radio` components that are wrapped in a
-`<fieldset>` element. The `<fieldset>` element renders a `<legend>` element that
+The `RadioGroup` renders a group of `Radio` components along with a title that
 can be visually hidden through the `showLabel` prop.
 
 Resources:

diff --git a/src/components/RadioGroup/RadioGroup.test.tsx b/src/components/RadioGroup/RadioGroup.test.tsx
@@ -59,7 +59,7 @@ describe("Radio Accessibility", () => {
     );
     expect(await axe(container)).toHaveNoViolations();
   });
-  it("passes axe accessibility with the legend hidden", async () => {
+  it("passes axe accessibility with the title hidden", async () => {
     const { container } = render(
       <RadioGroup
         id="radioGroup"
@@ -93,7 +93,7 @@ describe("Radio Button", () => {
     expect(screen.getByText("Radio 4")).toBeInTheDocument();
   });
 
-  it("<legend> element is available in the DOM when 'showLabel' prop is set to true or false", () => {
+  it("span element is available in the DOM when 'showLabel' prop is set to true or false", () => {
     const { container, rerender } = render(
       <RadioGroup
         id="radioGroup"
@@ -106,8 +106,10 @@ describe("Radio Button", () => {
         <Radio id="radio4" value="4" labelText="Radio 4" />
       </RadioGroup>
     );
-    expect(container.querySelector("legend")).toBeVisible();
-    expect(container.querySelector("legend")).toHaveTextContent("Test Label");
+    expect(container.querySelectorAll("span")[0]).toBeVisible();
+    expect(container.querySelectorAll("span")[0]).toHaveTextContent(
+      "Test Label"
+    );
 
     rerender(
       <RadioGroup
@@ -121,8 +123,10 @@ describe("Radio Button", () => {
         <Radio id="radio4" value="4" labelText="Radio 4" />
       </RadioGroup>
     );
-    expect(container.querySelector("legend")).toBeVisible();
-    expect(container.querySelector("legend")).toHaveTextContent("Test Label");
+    expect(container.querySelectorAll("span")[0]).toBeVisible();
+    expect(container.querySelectorAll("span")[0]).toHaveTextContent(
+      "Test Label"
+    );
   });
 
   it("renders visible helper or error text", () => {
@@ -166,20 +170,6 @@ describe("Radio Button", () => {
     ).not.toBeInTheDocument();
   });
 
-  it("sets the RadioGroup's ID", () => {
-    render(
-      <RadioGroup labelText="Test Label" name="test5" id="some-id">
-        <Radio id="radio2" value="2" labelText="Radio 2" />
-      </RadioGroup>
-    );
-
-    // The "group" role here is for the `fieldset` element.
-    expect(screen.getByRole("group")).toHaveAttribute(
-      "id",
-      "radio-group-some-id"
-    );
-  });
-
   it("sets the next value through the onChange function", () => {
     let newValue = "";
     const onChange = (value: string) => {
@@ -292,6 +282,45 @@ describe("Radio Button", () => {
     expect(screen.queryByText("There is an error :(")).not.toBeInTheDocument();
   });
 
+  it("should throw warning when a non-Radio component is used as a child", () => {
+    const warn = jest.spyOn(console, "warn");
+    render(
+      <RadioGroup labelText="wrong child!" name="wrong" id="wrong-child">
+        <p>This is wrong!</p>
+      </RadioGroup>
+    );
+    expect(warn).toHaveBeenCalledWith(
+      "NYPL Reservoir RadioGroup: Only `Radio` components are allowed inside " +
+        "the `RadioGroup` component."
+    );
+  });
+
+  it("logs a warning when there is no `id` passed", () => {
+    const warn = jest.spyOn(console, "warn");
+    render(
+      // @ts-ignore: Typescript complains when a required prop is not passed, but
+      // here we don't want to pass the required prop to make sure the warning appears.
+      <RadioGroup labelText="RadioGroup example" name="a11y-test">
+        <Radio id="radio1" value="1" labelText="Radio 1" />
+      </RadioGroup>
+    );
+    expect(warn).toHaveBeenCalledWith(
+      "NYPL Reservoir RadioGroup: This component's required `id` prop was not passed."
+    );
+  });
+
+  it("passes a ref to the input element", () => {
+    const ref = React.createRef<HTMLDivElement>();
+    render(
+      <RadioGroup id="column" labelText="column" name="column" ref={ref}>
+        <Radio value="2" labelText="Radio 2" id="radio-2" />
+        <Radio value="3" labelText="Radio 3" id="radio-3" />
+      </RadioGroup>
+    );
+
+    expect(screen.getByRole("radiogroup")).toBe(ref.current);
+  });
+
   it("renders the UI snapshot correctly", () => {
     const column = renderer
       .create(
@@ -470,43 +499,4 @@ describe("Radio Button", () => {
     expect(withChakraProps).toMatchSnapshot();
     expect(withOtherProps).toMatchSnapshot();
   });
-
-  it("should throw warning when a non-Radio component is used as a child", () => {
-    const warn = jest.spyOn(console, "warn");
-    render(
-      <RadioGroup labelText="wrong child!" name="wrong" id="wrong-child">
-        <p>This is wrong!</p>
-      </RadioGroup>
-    );
-    expect(warn).toHaveBeenCalledWith(
-      "NYPL Reservoir RadioGroup: Only `Radio` components are allowed inside " +
-        "the `RadioGroup` component."
-    );
-  });
-
-  it("logs a warning when there is no `id` passed", () => {
-    const warn = jest.spyOn(console, "warn");
-    render(
-      // @ts-ignore: Typescript complains when a required prop is not passed, but
-      // here we don't want to pass the required prop to make sure the warning appears.
-      <RadioGroup labelText="RadioGroup example" name="a11y-test">
-        <Radio id="radio1" value="1" labelText="Radio 1" />
-      </RadioGroup>
-    );
-    expect(warn).toHaveBeenCalledWith(
-      "NYPL Reservoir RadioGroup: This component's required `id` prop was not passed."
-    );
-  });
-
-  it("passes a ref to the input element", () => {
-    const ref = React.createRef<HTMLDivElement>();
-    const { container } = render(
-      <RadioGroup id="column" labelText="column" name="column" ref={ref}>
-        <Radio value="2" labelText="Radio 2" id="radio-2" />
-        <Radio value="3" labelText="Radio 3" id="radio-3" />
-      </RadioGroup>
-    );
-
-    expect(container.querySelector("fieldset > div")).toBe(ref.current);
-  });
 });
diff --git a/src/components/RadioGroup/RadioGroup.tsx b/src/components/RadioGroup/RadioGroup.tsx
@@ -1,4 +1,5 @@
 import {
+  Box,
   chakra,
   ChakraComponent,
   RadioGroup as ChakraRadioGroup,
@@ -7,7 +8,6 @@ import {
 } from "@chakra-ui/react";
 import React, { forwardRef } from "react";
 
-import Fieldset from "../Fieldset/Fieldset";
 import HelperErrorText, {
   HelperErrorTextType,
 } from "../HelperErrorText/HelperErrorText";
@@ -57,9 +57,9 @@ export interface RadioGroupProps {
 const noop = () => {};
 
 /**
- * RadioGroup is a wrapper for DS `Radio` components that renders as a fieldset
- * HTML element along with optional helper text. The `name` prop is essential
- * for this form group element and is not needed for individual DS `Radio`
+ * `RadioGroup` is a wrapper for DS `Radio` components that render together
+ * along with an optional helper text. The `name` prop is essential for this
+ * form group element and is not needed for individual DS `Radio`
  * components when `RadioGroup` is used.
  */
 export const RadioGroup: ChakraComponent<
@@ -98,7 +98,10 @@ export const RadioGroup: ChakraComponent<
       const spacingProp = layout === "column" ? spacing.s : spacing.l;
       const newChildren: JSX.Element[] = [];
       // Get the Chakra-based styles for the custom elements in this component.
-      const styles = useMultiStyleConfig("RadioGroup", { isFullWidth });
+      const styles = useMultiStyleConfig("RadioGroup", {
+        isFullWidth,
+        isLegendHidden: !showLabel,
+      });
       // Props for the `ChakraRadioGroup` component.
       const radioGroupProps = {
         name,
@@ -145,16 +148,16 @@ export const RadioGroup: ChakraComponent<
       );
 
       return (
-        <Fieldset
+        <Box
           className={className}
           id={`radio-group-${id}`}
-          isLegendHidden={!showLabel}
-          isRequired={isRequired}
-          legendText={labelText}
-          showRequiredLabel={showRequiredLabel}
           {...rest}
           __css={styles}
         >
+          <Box as="span" __css={styles.spanLegend}>
+            {labelText}
+            {showRequiredLabel && isRequired && <span> (required)</span>}
+          </Box>
           <ChakraRadioGroup {...radioGroupProps}>
             <Stack direction={[layout]} spacing={spacingProp}>
               {newChildren}
@@ -167,7 +170,7 @@ export const RadioGroup: ChakraComponent<
             text={footnote}
             __css={styles.helperErrorText}
           />
-        </Fieldset>
+        </Box>
       );
     }
   )