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.
There was a problem hiding this comment.
Removed legacy approach with children. Removed all NewDialog* examples and replaced older examples with new implementation.
| 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.
There was a problem hiding this comment.
That's what I wanted to achieve. I can see in the guidelines that Title must be always centered when Icon is visible. Would you like to drop this restriction?
| * 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.
There was a problem hiding this comment.
Guidelines say that content and actions are not optional. Should we override it and allow title-only dialogs? DialogWithLoadingIndicator.tsx uses an empty array for actions but in this case it's against the guidelines and shouldn't be used this way. Do we still want content and actions as 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.
There was a problem hiding this comment.
This was a proposed example for actions and it looked solid but you're right that it cannot express a custom action component.
I changed actions into React.ReactNode[] type which results in example usage:
actions={[
<Button key="disagree-btn" onPress={close} textColor={Palette.error50}>
Disagree
</Button>,
<Button key="agree-btn" onPress={close}>
Agree
</Button>,
]}
This way we simplify a composition and let developers decide what action component they would like to use.
I could still change it to React.ReactNode instead and we would have these possibilities:
- Developers to keep using
Dialog.Actionswhich does not simplify usingDialogcomponent astitleorcontentprops.
actions={
<Dialog.Actions>
<Button onPress={close} textColor={Palette.error50}>
Disagree
</Button>
<Button onPress={close}>
Agree
</Button>
</Dialog.Actions>
]}
- We would accept a
React.ReactNodebut it would add additional logic to theDialogActions.tsxto unwrap one level of children to properly assign a styling to them. Each child is supposed to havemarginRight: 8expect the last one.
actions={
<>
<Button onPress={close} textColor={Palette.error50}>
Disagree
</Button>
<Button onPress={close}>
Agree
</Button>
</>
]}
What do you think?
| ) : 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.
There was a problem hiding this comment.
Ignoring as actions changed a type.
| 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.
There was a problem hiding this comment.
Added accessibilityLabel as a Dialog prop to enable passing custom aria-label. I also added a condition where if title is a string then it should be taken as aria-label by default which is what guidelines suggest. We could also drop it at only pass a provided accessibilityLabel. What do you think?
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.