-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Dialog] DialogTitle overrides DialogContent className #26795
Comments
I didn't make that particular change nor do I think it should stay. It makes customization not straight forward. You'd have to ask @oliviertassinari what problem the sibling selector solves and what alternatives he tried. |
The biggest constraint I had was the lack of support for
The least worse solution I could find is: |
For the solution, an update of the docs like proposed by @mnajdova sounds great 👍 diff --git a/docs/src/pages/guides/migration-v4/migration-v4.md b/docs/src/pages/guides/migration-v4/migration-v4.md
index b66b55e80d..2d8629a8be 100644
--- a/docs/src/pages/guides/migration-v4/migration-v4.md
+++ b/docs/src/pages/guides/migration-v4/migration-v4.md
@@ -773,6 +773,10 @@ You can use the [`collapse-rename-collapsedheight` codemod](https://github.com/m
</Typography>
```
+ Note that the DialogContent now applies its padding with a specificity of 2.
+ It was changed to work around the lack of [support of `:first-child`](https://github.com/emotion-js/emotion/issues/1178) in emotion.
+ See [#26795](https://github.com/mui-org/material-ui/issues/) for more details.
+
### Divider
- Use border instead of background color. It prevents inconsistent height on scaled screens. |
Why can't we apply the styles to the DialogContent? A Dialog should always have a heading anyway. Instead, people should add padding if they purposely omit the heading. |
@eps1lon It's written as is it was a fact. Why should a DialogTitle be always rendered? Material Design doesn't seem to support this affirmation.
https://material.io/components/dialogs#usage Or the first demo of the page. It seems that they are consistent with the legacy version: https://material.io/archive/guidelines/components/dialogs.html#dialogs-behavior
Actually, we could likely leverage that a dialog title will most often be used to reverse the CSS tradeoff. |
I have tried: diff --git a/packages/material-ui/src/DialogContent/DialogContent.js b/packages/material-ui/src/DialogContent/DialogContent.js
index 131e5e6980..0ed698d896 100644
--- a/packages/material-ui/src/DialogContent/DialogContent.js
+++ b/packages/material-ui/src/DialogContent/DialogContent.js
@@ -32,7 +32,7 @@ const DialogContentRoot = styled('div', {
// Add iOS momentum scrolling for iOS < 13.0
WebkitOverflowScrolling: 'touch',
overflowY: 'auto',
- padding: '20px 24px',
+ padding: '0 24px 20px',
...(styleProps.dividers
? {
padding: '16px 24px',
@@ -40,8 +40,8 @@ const DialogContentRoot = styled('div', {
borderBottom: `1px solid ${theme.palette.divider}`,
}
: {
- '.MuiDialogTitle-root + &': {
- paddingTop: 0,
+ ':not(.MuiDialogTitle-root) + &': {
+ paddingTop: 20,
},
}),
})); But it's not working. How about we add a new prop like |
A dialog having no title is problematic UI. Customization should not be our concern in this case. Otherwise we encourage problematic UIs. |
Looking at WAI-ARIA, they seems to support that a dialog should always be labeled.
https://www.w3.org/TR/wai-aria-practices-1.1/ We could support
@eps1lon This is confusing me. Do you mean from a DOM perspective or a design perspective? The two seems distinct. Also, it seems exposed as an axiom, adding resources to support it would help. |
I don't know what a "DOM perspective" is. The DOM is just a programming interface. I'm talking about a design perspective. Just as your documents should have titles. Dialogs are mini-documents that are nested within other documents. Especially for fullscreen dialogs I don't see how a dialog without a title is a good thing.
"Content" is not the heading or title and abusing it as such is just straight up confusing. Consider how you would teach this. |
@eps1lon OK thanks for the clarification, so it's none of the option concepts I was wondering about (DOM, as the a11y tree, and design as the Material Design aspect. Your point is more conceptual, "title" as something that labels the content of the modal, which is close to the a11y that requires a label. I agree. Here Material Design is saying that the "title" can either be the "DialogTitle" or "DialogContent" (if alone). |
I didn't find any such statement. I even found statements contradicting this claim:
-- https://material.io/components/dialogs#anatomy
-- https://material.io/components/dialogs#behavior The only statement that comes close is only applicable under certain conditions:
-- https://material.io/components/dialogs#full-screen-dialog And then people can wire the dialog label up differently. |
@eps1lon From what I understand of the logic of Material Design: The DialogTitle + DialogContent is visually harmful with very short dialogs, e.g. quick confirmation prompts. Only rendering a DialogTitle wouldn't match their design expectations, and rendering a DialogTitle + DialogContent with the same content would be repetitive, making the action harder for the end-users. Hence why they have made the DialogTitle optional.
@eps1lon I come to this conclusion (DialogContent can act as a title when DialogTitle is not rendered) from:
We can label the DialogContent (design) as the title (a11y label), problem solved.
Developers should use a DialogTitle if they need a scrollbar, it doesn't contradict that DialogContent can act as the a11y label of the modal. |
A real-life example in Google's products (GDrive) Can also be found in YouTube. For the solution of this GitHub issue. My proposal would be, built on top of #26814: diff --git a/packages/material-ui/src/DialogContent/DialogContent.js b/packages/material-ui/src/DialogContent/DialogContent.js
index 131e5e6980..5ecfb34f7c 100644
--- a/packages/material-ui/src/DialogContent/DialogContent.js
+++ b/packages/material-ui/src/DialogContent/DialogContent.js
@@ -5,6 +5,7 @@ import { unstable_composeClasses as composeClasses } from '@material-ui/unstyled
import styled from '../styles/styled';
import useThemeProps from '../styles/useThemeProps';
import { getDialogContentUtilityClass } from './dialogContentClasses';
+import DialogContext from '../Dialog/DialogContext';
const useUtilityClasses = (styleProps) => {
const { classes, dividers } = styleProps;
@@ -32,18 +33,16 @@ const DialogContentRoot = styled('div', {
// Add iOS momentum scrolling for iOS < 13.0
WebkitOverflowScrolling: 'touch',
overflowY: 'auto',
- padding: '20px 24px',
+ padding: '0 24px 20px',
...(styleProps.dividers
- ? {
- padding: '16px 24px',
- borderTop: `1px solid ${theme.palette.divider}`,
- borderBottom: `1px solid ${theme.palette.divider}`,
- }
- : {
- '.MuiDialogTitle-root + &': {
- paddingTop: 0,
- },
- }),
+ && {
+ padding: '16px 24px',
+ borderTop: `1px solid ${theme.palette.divider}`,
+ borderBottom: `1px solid ${theme.palette.divider}`,
+ }),
+ ...(styleProps.noTitle && {
+ paddingTop: 20
+ }),
}));
const DialogContent = React.forwardRef(function DialogContent(inProps, ref) {
@@ -52,15 +51,18 @@ const DialogContent = React.forwardRef(function DialogContent(inProps, ref) {
name: 'MuiDialogContent',
});
- const { className, dividers = false, ...other } = props;
- const styleProps = { ...props, dividers };
+ const { className, dividers = false, noTitle = false, id: idProp, ...other } = props;
+ const styleProps = { ...props, noTitle, dividers };
const classes = useUtilityClasses(styleProps);
+ const { titleId: id = idProp } = React.useContext(DialogContext);
+
return (
<DialogContentRoot
className={clsx(classes.root, className)}
styleProps={styleProps}
ref={ref}
+ id={noTitle ? id : undefined}
{...other}
/>
); This would solve the issue of @jlin5 by always having a flat level of specificity, making overrides easier. It also resonates with the point of @eps1lon around how the usage of the DialogTitle + DialogContent should be a lot more frequent. diff --git a/docs/src/pages/components/dialogs/AlertDialog.tsx b/docs/src/pages/components/dialogs/AlertDialog.tsx
index d06b6c6860..0b79f9ed9d 100644
--- a/docs/src/pages/components/dialogs/AlertDialog.tsx
+++ b/docs/src/pages/components/dialogs/AlertDialog.tsx
@@ -28,14 +28,8 @@ export default function AlertDialog() {
aria-labelledby="alert-dialog-title"
aria-describedby="alert-dialog-description"
>
- <DialogTitle id="alert-dialog-title">
+ <DialogContent noTitle>
{"Use Google's location service?"}
- </DialogTitle>
- <DialogContent>
- <DialogContentText id="alert-dialog-description">
- Let Google help apps determine location. This means sending anonymous
- location data to Google, even when no apps are running.
- </DialogContentText>
</DialogContent>
<DialogActions>
<Button onClick={handleClose}>Disagree</Button> Before After |
I don't understand why you're hell bent on making no title work when there's a really simple solution to the problem that puts the usability of the UI first: just add a title. Instead we have to overload the DialogContent component which makes implementation harder and makes in near impossible to explain to a developer what this component does or how you should use it. It's sacrificing the usability of the component and the created UI just because some stakeholder told you they really don't want to change how they work? |
With the current behavior, an outlined |
Have the same issue. Title gets rid of the top padding for me on Dialog Content. Trying to add paddingTop to Dialog Content and it gets overridden by:
This is on "@material-ui/core": "^5.0.0-beta.1". This is an issue if you have the whole title background a different color then the content. Content will bump right up to it. Example: https://codesandbox.io/s/hopeful-fermi-0v850?file=/src/App.js:514-515 |
I just came to this issue after upgrading from v4 to v5. We have many dialogs where the first TextField label gets truncated as shared by @amaslakov above. |
Same for me like the previous replies - any TextField or Autocomplete's floating label gets truncated, the upper ~50% part gets to "disappear" by the DialogTitle's bottom padding (or by the "mistake" of the missing top padding of DialogContent). My opinion is that we should always have the DialogContent's top padding -> it would solve this issue + would work for dialogs without a DialogTitle. |
Was there a specific reason why this DialogTitle component didn't get the same prop system similar to But since I could always use This applies to The use case is;
MUI V4 |
I'm currently experiencing this same issue on @mui/[email protected] Any I see that this thread hasn't had a reply in awhile. Has anybody been able to resolve this or find a work around? |
We can't change anything for now, at least until the next major. I would propose bumping the specificity using |
To solve the problem of the floating labels getting cut of I'm using this for now: You could also set |
I have fixed this by wrapping |
worked for me , but still very hacky. |
As specified by @mnajdova here, best implementation for now would be adding style to parent Dialog component ,
![]() |
Current Behavior 😯
The DialogTitle className (MuiDialogTitle-root) is overriding the set className to DialogContent.
Expected Behavior 🤔
The DialogTitle className should not override the set className to DialogContent.
Steps to Reproduce 🕹
https://codesandbox.io/s/sparkling-architecture-47w73?file=/src/index.tsx
This is happening after @material-ui/[email protected]
To reproduce, change the package dependency in the codepen @material-ui/core to 5.0.0-alpha.36 or higher.
The text was updated successfully, but these errors were encountered: