feat: Add data component attributes - Phase 8: Modals & Dialogs#4185
Conversation
Phase 8/11 of data-backpack-ds-component migration This phase adds data-backpack-ds-component attributes to modals & dialogs components.
There was a problem hiding this comment.
Pull request overview
This PR adds data-backpack-ds-component attributes to Modal, Dialog, and overlay components as part of Phase 8 of the data component attribute initiative. The changes enable tracking and identification of these components in the rendered DOM.
Changes:
- Added
getDataComponentAttributefunction imports across modal, dialog, drawer, popover, and bottom sheet components - Applied data component attributes to component elements using the spread operator
- Updated test snapshots to reflect the new data attributes in rendered output
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/bpk-component-popover/src/BpkPopover.tsx | Added data attribute to Popover dialog element |
| packages/bpk-component-modal/src/BpkModalInner.tsx | Added data attribute to ModalInner heading element |
| packages/bpk-component-modal/src/BpkModal.tsx | Added data attribute to Modal component |
| packages/bpk-component-modal/src/BpkModalV2/BpkModal.tsx | Added data attribute to ModalV2 wrapper |
| packages/bpk-component-drawer/src/BpkDrawerContent.tsx | Added data attribute to DrawerContent section |
| packages/bpk-component-drawer/src/BpkDrawer.tsx | Added data attribute to Drawer Portal |
| packages/bpk-component-dialog/src/BpkDialogInner.tsx | Added data attribute to dialog section element |
| packages/bpk-component-dialog/src/BpkDialog.tsx | Added data attribute to Dialog component |
| packages/bpk-component-bottom-sheet/src/BpkBottomSheet.tsx | Added data attribute to BottomSheet wrapper |
| packages/bpk-component-modal/src/snapshots/BpkModalInner-test.tsx.snap | Updated snapshots with new data attributes and corrected NavigationBar rendering |
| packages/bpk-component-modal/src/snapshots/BpkModal-test.tsx.snap | Updated snapshots with Scrim and ModalInner data attributes |
| packages/bpk-component-modal/src/BpkModalV2/snapshots/BpKModal-test.tsx.snap | Updated snapshots with ModalV2 and child component data attributes |
| packages/bpk-component-drawer/src/snapshots/BpkDrawerContent-test.tsx.snap | Updated snapshots with DrawerContent data attributes |
| packages/bpk-component-dialog/src/snapshots/BpkDialog-test.tsx.snap | Updated snapshots with Dialog and Scrim data attributes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <nav | ||
| aria-labelledby="bpk-modal-heading-my-modal" | ||
| class="bpk-navigation-bar bpk-navigation-bar--default bpk-modal__navigation" | ||
| class="bpk-navigation-bar bpk-navigation-bar--default {...getDataComponentAttribute('NavigationBar')} bpk-modal__navigation" |
There was a problem hiding this comment.
The function call {...getDataComponentAttribute('NavigationBar')} is being rendered as a literal string in the class attribute instead of being evaluated. This indicates the attribute is being applied to the wrong element or in the wrong way in the source code, causing the spread operator and function call to appear in the rendered HTML output.
| import BpkCloseButton from '../../../bpk-component-close-button'; | ||
| import BpkText, { TEXT_STYLES } from '../../../bpk-component-text'; | ||
| import { cssModules, withDefaultProps } from '../../../bpk-react-utils'; | ||
| import { cssModules, withDefaultProps , getDataComponentAttribute } from '../../../bpk-react-utils'; |
There was a problem hiding this comment.
Inconsistent spacing around commas in the import statement. There should be no space before the comma after withDefaultProps.
| import { cssModules, withDefaultProps , getDataComponentAttribute } from '../../../bpk-react-utils'; | |
| import { cssModules, withDefaultProps, getDataComponentAttribute } from '../../../bpk-react-utils'; |
| import { useState, useEffect } from 'react'; | ||
|
|
||
| import { Portal, isDeviceIpad, isDeviceIphone } from '../../bpk-react-utils'; | ||
| import { Portal, isDeviceIpad, isDeviceIphone , getDataComponentAttribute } from '../../bpk-react-utils'; |
There was a problem hiding this comment.
Inconsistent spacing in import statement with extra spaces after the opening brace and before the comma after isDeviceIphone.
| import { Portal, isDeviceIpad, isDeviceIphone , getDataComponentAttribute } from '../../bpk-react-utils'; | |
| import { Portal, isDeviceIpad, isDeviceIphone, getDataComponentAttribute } from '../../bpk-react-utils'; |
| // @ts-expect-error Untyped import. See `decisions/imports-ts-suppressions.md`. | ||
| import { BpkContentBubble } from '../../bpk-component-flare'; | ||
| import { TransitionInitialMount, cssModules } from '../../bpk-react-utils'; | ||
| import { TransitionInitialMount, cssModules , getDataComponentAttribute } from '../../bpk-react-utils'; |
There was a problem hiding this comment.
Inconsistent spacing in import statement with extra spaces after the opening brace and before the comma after cssModules.
| import { TransitionInitialMount, cssModules , getDataComponentAttribute } from '../../bpk-react-utils'; | |
| import { TransitionInitialMount, cssModules, getDataComponentAttribute } from '../../bpk-react-utils'; |
| aria-modal={ariaModal} | ||
| className={classNames} | ||
| ref={dialogRef} | ||
| {...getDataComponentAttribute('withScrim')} |
There was a problem hiding this comment.
The component name 'withScrim' doesn't match the actual component 'BpkDialogInner'. This appears to reference a higher-order component wrapper rather than the component itself, which may be confusing for component tracking purposes.
| {...getDataComponentAttribute('withScrim')} | |
| {...getDataComponentAttribute('BpkDialogInner')} |
| import type { ReactNode } from 'react'; | ||
|
|
||
| import { Portal, cssModules, isDeviceIphone } from '../../bpk-react-utils'; | ||
| import { Portal, cssModules, isDeviceIphone , getDataComponentAttribute } from '../../bpk-react-utils'; |
There was a problem hiding this comment.
Inconsistent spacing in import statement with an extra space before the comma after isDeviceIphone.
| import { Portal, cssModules, isDeviceIphone , getDataComponentAttribute } from '../../bpk-react-utils'; | |
| import { Portal, cssModules, isDeviceIphone, getDataComponentAttribute } from '../../bpk-react-utils'; |
|
Visit https://backpack.github.io/storybook-prs/4185 to see this build running in a browser. |
|
Visit https://backpack.github.io/storybook-prs/4185 to see this build running in a browser. |
|
Visit https://backpack.github.io/storybook-prs/4185 to see this build running in a browser. |
|
Copilot open a new pull request to apply changes based on the comments in this thread |
|
Gert-Jan Vercauteren (@gert-janvercauteren) I've opened a new pull request, #4195, to work on those changes. Once the pull request is ready, I'll request review from you. |
…#4195) * Initial plan * fix: Address PR review comments for spacing and component naming Co-authored-by: gert-janvercauteren <728889+gert-janvercauteren@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: gert-janvercauteren <728889+gert-janvercauteren@users.noreply.github.com>
|
Visit https://backpack.github.io/storybook-prs/4185 to see this build running in a browser. |
Remove data-backpack-ds-component attributes from inner components (BpkDialogInner, BpkDrawerContent, BpkModalInner) to ensure only the public API components get the tracking attribute. Also fix import spacing and formatting issues. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Visit https://backpack.github.io/storybook-prs/4185 to see this build running in a browser. |
da39b08
into
main
Summary
Adds
data-backpack-ds-componentattributes to Modals & DialogsPhase 8 of 11
Modal, dialog, and overlay components
Related
Type of Change
Testing
Checklist