feat(dialog): add dialog header component - FE-7750 - #8128
Conversation
79da4a3 to
abc5558
Compare
| @@ -264,7 +264,7 @@ const StyledDialogTitle = styled.div<StyledDialogTitleProps>` | |||
|
|
|||
| [data-element="dialog-title-help-wrapper"] { | |||
There was a problem hiding this comment.
Markup using dialog-title-help-wrapper is removed, so this selector looks dead, isn't it?
| <Icon | ||
| type={iconType} | ||
| color={color} | ||
| fontSize="medium" |
There was a problem hiding this comment.
Check if fontSize is deprecated
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Replaces the Dialog heading HOC with an exported DialogHeader component and adds status-header accessibility support.
Changes:
- Adds status variants, exports, stories, and documentation.
- Adds ARIA ID coordination between
DialogandDialogHeader. - Updates tests and disables the deprecated
helpbehavior.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/components/dialog/index.ts |
Exports the new header API. |
src/components/dialog/dialog.test.tsx |
Updates deprecated-help coverage. |
src/components/dialog/dialog.stories.tsx |
Adds status-header stories. |
src/components/dialog/dialog.pw.tsx |
Updates help and accessibility tests. |
src/components/dialog/dialog.mdx |
Documents status headers. |
src/components/dialog/dialog.component.tsx |
Re-exports header APIs. |
src/components/dialog/__internal__/__next__/index.ts |
Exports internal header APIs. |
src/components/dialog/__internal__/__next__/dialog.test.tsx |
Tests ARIA integration. |
src/components/dialog/__internal__/__next__/dialog.style.ts |
Adjusts header styling. |
src/components/dialog/__internal__/__next__/dialog.pw.tsx |
Adds no-subtitle accessibility coverage. |
src/components/dialog/__internal__/__next__/dialog.component.tsx |
Integrates DialogHeader. |
src/components/dialog/__internal__/__next__/dialog-test.stories.tsx |
Migrates HOC stories. |
src/components/dialog/__internal__/__next__/dialog-header/dialog-header.test.tsx |
Tests the new component. |
src/components/dialog/__internal__/__next__/dialog-header/dialog-header.component.tsx |
Implements DialogHeader. |
src/components/dialog/__internal__/__next__/components-test.pw.tsx |
Migrates Playwright fixtures. |
skills/carbon-react/components/alert.md |
Excluded documentation update. |
skills/carbon-react/components/dialog.md |
Excluded documentation update. |
Files excluded by content exclusion policy (2)
- skills/carbon-react/components/alert.md
- skills/carbon-react/components/dialog.md
Suppressed comments (1)
src/components/dialog/internal/next/dialog.component.tsx:353
- The generated title ID overrides an explicit
aria-labelledby, even thoughDialogPropsdocuments that prop for custom React-node titles and the previous HOC preserved it. Consumers can no longer supply a different accessible label when usingDialogHeader; give the explicit prop precedence.
"aria-labelledby":
dialogHeaderTitleId ||
(title && typeof title === "string" ? titleId : ariaLabelledBy),
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| "aria-describedby": | ||
| subtitle && typeof subtitle === "string" ? subtitleId : ariaDescribedBy, | ||
| dialogHeaderSubtitleId || | ||
| (subtitle && typeof subtitle === "string" | ||
| ? subtitleId | ||
| : ariaDescribedBy), |
| /** | ||
| * Adds Help tooltip to Header. | ||
| * @deprecated This prop no longer has any effect and will be removed in a future release. | ||
| */ |
| open | ||
| title={ | ||
| <DialogHeader | ||
| title="Dialog with status" |
| ); | ||
| passSubtitle = false; | ||
| // Always call hooks unconditionally at the top level | ||
| const generatedTitleId = useRef(createGuid()).current; |
There was a problem hiding this comment.
| const generatedTitleId = useRef(createGuid()).current; | |
| const generatedTitleId = useRef(context?.titleId || createGuid()).current; |
|
|
||
| /** Allowed status variants for the dialog heading icon. */ | ||
| export type DialogHeadingStatus = | ||
| export type DialogHeading = |
There was a problem hiding this comment.
| export type DialogHeading = | |
| export type DialogHeadingStatus = |
non-blocking, I think it was better when it was a bit more explicit
| const isDialogHeader = | ||
| title && | ||
| React.isValidElement(title) && | ||
| title.type === DialogHeadingStatus; |
There was a problem hiding this comment.
suggestion(non-blocking): Since DialogHeader is exported from Dialog subpath and documented for consumer use, consumer can easily wrap it in memo or styled component, This means title.type === DialogHeadingStatus can fail. Can we do better or create a follow-up ticket?
There was a problem hiding this comment.
I can make this change in this PR. It makes sense to do it now 👍🏻
| let dialogHeaderTitleId: string | undefined; | ||
| let dialogHeaderSubtitleId: string | undefined; | ||
|
|
||
| if (isDialogHeader && title && React.isValidElement(title)) { |
There was a problem hiding this comment.
suggestion: to avoid the casting we could do something like
const isValidDialogHeader = isDialogHeader && React.isValidElement<{ subtitle?: React.ReactNode }>(title);
| const dialogTitle = () => { | ||
| const renderTitle = ( | ||
| // Helper to render subtitle | ||
| const renderSubtitle = () => ( |
There was a problem hiding this comment.
suggestion (non-blocking): we could add a short circuit here and then remove the boolean logic below but either is fine
if (!subtitle) return;
| const renderTitle = isDialogHeader ? ( | ||
| <DialogHeadingStatusContext.Provider | ||
| value={{ | ||
| titleId: dialogHeaderTitleId, |
There was a problem hiding this comment.
suggestion: if you go with the above change you can probably just pass titleId and subtitleId via the provider. I could be wrong on this one though as it may need a conditional check instead of greedily passing etc
| @@ -317,10 +367,14 @@ export const Dialog = forwardRef<DialogHandle, DialogProps>( | |||
|
|
|||
| const ariaProps = { | |||
There was a problem hiding this comment.
suggestion: these are a bit complicated to read with nested ternaries
const ariaProps = {
"aria-describedby":
(isValidDialogHeader && title.props.subtitle) || (subtitle && typeof subtitle === "string")
? subtitleId
: ariaDescribedBy,
"aria-label": ariaLabel,
"aria-labelledby":
(isValidDialogHeader || (title && typeof title === "string")) ? titleId : ariaLabelledBy,
};
| ); | ||
|
|
||
| // Check if title is a DialogHeader component | ||
| const isDialogHeader = React.useMemo(() => { |
There was a problem hiding this comment.
suggestion: we could move this to some helper functions with typing to avoid the casting etc
// utils.ts
const canHaveProperties = (value: unknown): value is object =>
value !== null && (typeof value === "object" || typeof value === "function");
const hasDialogHeadingStatusMarker = (value: unknown) =>
canHaveProperties(value) &&
"$$carbonDialogHeadingStatus" in value && value.$$carbonDialogHeadingStatus;
export const isDialogHeadingStatusComponent = (componentType: unknown) =>
hasDialogHeadingStatusMarker(componentType) ||
(canHaveProperties(componentType) &&
"type" in componentType &&
hasDialogHeadingStatusMarker(componentType.type));
const isDialogHeader = React.useMemo(() => {
if (!title || !React.isValidElement(title)) {
return false;
}
// Check direct type match
if (title.type === DialogHeadingStatus) {
return true;
}
return isDialogHeadingStatusComponent(title.type);
}, [title]);
| title: React.ReactNode; | ||
| subtitle?: React.ReactNode; | ||
| status: DialogHeadingStatus; | ||
| } |
There was a problem hiding this comment.
suggestion: add the status interface here instead, and on line 55
const DialogHeadingStatus: DialogHeadingStatusComponent = forwardRef<
...
| DialogHeadingStatus.displayName = "DialogHeadingStatus"; | ||
|
|
||
| return Enhanced; | ||
| // Static marker to identify this component even when wrapped in memo/styled-components |
There was a problem hiding this comment.
suggestion: if you go with the above then end of file can just be
DialogHeadingStatus.$$carbonDialogHeadingStatus = true;
export default DialogHeadingStatus;
0c6530b
0c6530b to
73628b1
Compare
|
🎉 This PR is included in version 161.25.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Proposed behaviour
Refactor the HOC for heading variants.
Current behaviour
Currently we have a HOC for generating heading variants but this is overkill for what we need.
Checklist
d.tsfile added or updated if requiredQA
Additional context
N/A
Testing instructions
There should be no visual or functional regressions with the heading variants of Dialog.
There should also be no regression associated with Dialog in general too.