Skip to content

fix(detaillist/text): L3-10789 improve semantic HTML to remove accessibility violations with <dl and <label#771

Open
scottdickerson wants to merge 6 commits intomainfrom
L3-10789-accessibility-violations
Open

fix(detaillist/text): L3-10789 improve semantic HTML to remove accessibility violations with <dl and <label#771
scottdickerson wants to merge 6 commits intomainfrom
L3-10789-accessibility-violations

Conversation

@scottdickerson
Copy link
Contributor

@scottdickerson scottdickerson commented Nov 20, 2025

Jira ticket

L3-10789

Summary

This pull request refactors the DetailList component, moving it from the patterns directory to components, and updates its implementation to use a div instead of a dl element. It also adjusts related imports throughout the codebase and updates the Detail component to use div elements instead of dt/dd. Additionally, there are minor changes to the Text component's default element logic and its variants for TextVariants.label* so we don't render <label by default since we're now using that token for more than input labels. The <Input type components should always pass element='label' now for their labels.

Change List (describe the changes made to the files)

Component Refactoring and Relocation

  • Moved DetailList from src/patterns/DetailList/DetailList.tsx to src/components/DetailList/DetailList.tsx, and changed its top-level element from dl to div, updating its props and implementation accordingly. [1] [2] [3] [4]
  • Updated all imports and exports to reference DetailList from the new components location instead of patterns, including in src/index.ts, src/patterns/BidSnapshot/BidSnapshot.tsx, and src/patterns/ObjectTile/ObjectTile.tsx. [1] [2] [3] [4]
  • Updated SCSS imports to match the new location of DetailList. [1] [2]

Detail Component Updates

  • Refactored the Detail component to use div elements for labels and values instead of semantic dt/dd, and updated corresponding tests to match the new markup structure. [1] [2] [3]

Text Component Improvements

  • Changed the default HTML element for label variants in the Text component from label to span, and updated the related test and utility function. [1] [2]
  • Minor adjustment in DescriptiveRadioButton to explicitly set element="label" for label text.

Text Variants Reorganization

  • Reordered the declaration of new TextVariants in src/components/Text/types.ts for clarity; no functional changes to the available variants. [1] [2]

Acceptance Test (how to verify the PR)

  • Check the Accessibility addon inside storybook and confirm that many of the violations have been fixed

Regression Test

  • (Optional) Add verification steps to make sure this PR doesn't break old functionality

Evidence of testing

Before:
image

After
image

Things to look for during review

  • PR title should correctly describe the most significant type of commit. I.e. feat(scope): ... if a minor release should be triggered.
  • All commit messages follow convention and are appropriate for the changes
  • All references to phillips class prefix are using the prefix variable
  • All major areas have a data-testid attribute.
  • Document all props with jsdoc comments
  • All strings should be translatable.
  • Unit tests should be written and should have a coverage of 90% or higher in all areas.

New Components

  • Are there any accessibility considerations that need to be taken into account and tested?
  • Default story called "Playground" should be created for all new components
  • Create a jsdoc comment that has an Overview section and a link to the Figma design for the component
  • Export the component and its typescript type from the index.ts file
  • Import the component scss file into the componentStyles.scss file.

@netlify
Copy link

netlify bot commented Nov 20, 2025

Deploy Preview for phillips-seldon ready!

Name Link
🔨 Latest commit 0b9130b
🔍 Latest deploy log https://app.netlify.com/projects/phillips-seldon/deploys/691f7d7afe41cd000760e27b
😎 Deploy Preview https://deploy-preview-771--phillips-seldon.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@github-actions
Copy link

github-actions bot commented Nov 20, 2025

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the DetailList and Detail components to address accessibility violations by replacing semantic dictionary list elements (<dl>, <dt>, <dd>) with generic <div> elements, and changes the default HTML element for label text variants from <label> to <span> to prevent invalid nested label structures.

Key Changes:

  • Moved DetailList from src/patterns/ to src/components/ and changed it from <dl> to <div>
  • Updated Detail component to use <div> elements instead of <dt>/<dd>
  • Changed Text component's default element for label variants from <label> to <span>, requiring explicit element="label" when used with form inputs

Reviewed Changes

Copilot reviewed 11 out of 18 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/patterns/ObjectTile/ObjectTile.tsx Updated import path for DetailList from patterns to components
src/patterns/BidSnapshot/BidSnapshot.tsx Updated import path for DetailList from patterns to components
src/index.ts Updated exports to reference DetailList from components (contains duplicate export issue)
src/components/Text/utils.ts Changed default element for label variants from 'label' to 'span'
src/components/Text/utils.test.ts Updated test expectation and description for label variant
src/components/Text/types.ts Reordered TextVariants enum - moved new variants to top
src/components/DetailList/utils.tsx Added utility function for generating Detail component keys
src/components/DetailList/utils.test.tsx Added tests for getDetailKey utility (contains type error)
src/components/DetailList/types.ts Added DetailListAlignment enum definition
src/components/DetailList/index.ts Added barrel export for DetailList component
src/components/DetailList/_detailList.scss Added SCSS styles for DetailList component (contains selector typo)
src/components/DetailList/DetailList.tsx Moved component from patterns, changed dl to div, added utilities (has multiple issues)
src/components/DetailList/DetailList.test.tsx Added basic tests for DetailList (needs more coverage)
src/components/DetailList/DetailList.stories.tsx Added Storybook stories for DetailList (incorrect category path)
src/components/Detail/Detail.tsx Changed dt/dd elements to div elements
src/components/Detail/Detail.test.tsx Updated test queries to match new div structure
src/components/DescriptiveRadioButton/DescriptiveRadioButton.tsx Added explicit element="label" to Text component (creates invalid nested labels)
src/componentStyles.scss Moved DetailList SCSS import from patterns to components
Comments suppressed due to low confidence (3)

src/components/DetailList/DetailList.tsx:9

  • Remove this leftover scaffolding comment. The interface has already been updated to use ComponentProps<'div'>.
    src/components/DetailList/DetailList.tsx:35
  • The Storybook Link references patterns-detaillist but the component has been moved to components. Update the link to: https://phillips-seldon.netlify.app/?path=/docs/components-detaillist--overview
    src/components/DetailList/DetailList.tsx:55
  • Props are spread twice: {...commonProps} on line 48 and {...props} on line 55. This causes props to be duplicated and commonProps values to be overridden by props. Since commonProps already contains necessary props extracted by getCommonProps, remove the {...props} spread on line 55 to avoid unintended overrides.

scottdickerson and others added 4 commits November 20, 2025 14:41
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@scottdickerson scottdickerson added the DO_NOT_MERGE Either in feature freeze mode or not ready for merge label Dec 3, 2025
@netlify
Copy link

netlify bot commented Jan 15, 2026

Deploy Preview for phillips-seldon ready!

Name Link
🔨 Latest commit d0715ce
🔍 Latest deploy log https://app.netlify.com/projects/phillips-seldon/deploys/696922f2b3036a0008f06f38
😎 Deploy Preview https://deploy-preview-771--phillips-seldon.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Copy link
Contributor

@adietrich3074 adietrich3074 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

DO_NOT_MERGE Either in feature freeze mode or not ready for merge Needs 2nd Reviewer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants