refactor(dialog): MD3 updates, updated Dialog API - #5072
Conversation
|
|
||
| const DIALOG_ELEVATION: number = 24; | ||
|
|
||
| const renderChildren = (children: React.ReactNode) => { |
There was a problem hiding this comment.
React.Children + cloneElement is the composition problem v6 is meant to remove (#4954), and this adds child.type === sniffing on top. On the prop path you build the array yourself - pass the margin and centring down as props instead.
| children?: never; | ||
| }; | ||
|
|
||
| type LegacyProps = { |
There was a problem hiding this comment.
This leaves v6 with two Dialog APIs - children deprecated but kept, plus seven duplicated NewDialog* example screens. Which one ships? A major shouldn't carry a deprecation shim, but dropping children is a break worth deciding deliberately.
| const { colors } = theme; | ||
| const borderStyles = { | ||
| borderColor: colors.surfaceVariant, | ||
| borderColor: colors.outline, |
There was a problem hiding this comment.
- borderColor: colors.outline,
+ borderColor: colors.outlineVariant,md.comp.dialog.with-divider.divider.color is deprecated in favour of md.comp.divider.color - outline-variant, which is what Divider already uses.
|
|
||
| if (topMarginStyle || titleAlignmentStyle) { | ||
| return React.cloneElement(child, { | ||
| style: [topMarginStyle, child.props.style, titleAlignmentStyle], |
There was a problem hiding this comment.
- style: [topMarginStyle, child.props.style, titleAlignmentStyle],
+ style: [topMarginStyle, titleAlignmentStyle, child.props.style],As written, <Dialog.Title style={{ textAlign: 'left' }}> stops working once there's an icon.
| * Content of the dialog. Non-empty strings are rendered as Material 3 | ||
| * supporting text. | ||
| */ | ||
| content: React.ReactNode; |
There was a problem hiding this comment.
content and actions are required, so a title-only dialog won't typecheck and NewDialogWithLoadingIndicator.tsx:35 has to pass actions={[]}. Make both optional.
| testID?: string; | ||
| }; | ||
|
|
||
| type DialogActionsProps = Omit<ButtonProps, 'children'> & { label: string }; |
There was a problem hiding this comment.
Why { label }[] rather than actions?: React.ReactNode? An array of Button props can't express a custom action component, and it's a second shape to keep in sync with Button.
| ) : null; | ||
|
|
||
| return [dialogIcon, dialogTitle, dialogContent, dialogActions]; | ||
| }, [children, props, theme.colors.onSurfaceVariant]); |
There was a problem hiding this comment.
props is a fresh rest object every render, so this useMemo never hits. Drop it, or list the individual props.
| @@ -5206,10 +5206,10 @@ | |||
| "Dialog/Dialog": { | |||
There was a problem hiding this comment.
Run yarn docs generate - componentDocs6x.json and the .mdx pages are generated, and the JSDoc you added to DialogActions, DialogContent, DialogScrollArea and DialogTitle hasn't reached either. The hand-typed ### children (depracated…) heading will be overwritten.
| {actions.map( | ||
| ({ label, onPress: onActionPress, ...buttonProps }, index) => ( | ||
| <Button | ||
| key={index} |
There was a problem hiding this comment.
key={index} on a consumer-supplied array - the JSDoc note "Keep their order stable between renders" is working around it. Key on label.
| const dialogIcon = icon ? ( | ||
| <DialogIcon icon={icon} key="dialogIcon" /> | ||
| ) : null; | ||
| const dialogTitle = title ? <DialogTitle>{title}</DialogTitle> : null; |
There was a problem hiding this comment.
The new API knows the title, but nothing gives the dialog an accessible name - Modal sets aria-modal with no label. Worth wiring title through while the API is being designed.
2 participants
Motivation
The goal of this PR is to address Material Design 3 guidelines for the Dialog component based on the official documentation as well as this issue. This PR also provides ability to avoid using previous compound components implementation in favor of props directly passed to the Dialog component.
Changes
Material Design 3:
Dialog.ScrollAreadivider border color usesoutline,Dialoghas now restricted minimum and maximum width set to280dpand560dp,DialogIconis the first component in theDialogand it had a total top margin of48dpinstead of24dp,Dialognow detects ifDialog.IconandDialog.Titleare present and if so enforcingDialog.Titleto be centered.Dialog:
Dialogcomponent now supportsicon,title,content,actions,scrollableprops instead of using compound components.Dialogstill uses compound components underneath to properly comply with Material Design specs as these components enforce proper styling,Dialogto ensure the new approach works as intended.Example:
Dialogscreen within Example app with the same dialogs as before but using new API to make sure they look and work as expected.Related issue
Test plan
Reviewers can through new Dialogs added to the Example app. They are duplicated as previous dialogs but they use new approach.