[Dialog] Better mobile default margin?

[BernardA]

When I set the dialog as fullWidth={true} I expect it to take whole screen up to maxWidth.
On mobile, width is calculated as
width: calc(100% - 96px);, the 96px coming from the rule just below
margin: 48px;.
So, it actually does not take full width and it just looks bad.

• This is not a v0.x issue.
• I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior 🤔

Current Behavior 😯

It takes width minus margin, which in the case is 48px at each side ( well, top and bottom as well, but that does not matter)

Steps to Reproduce 🕹

Link: to sandbox

Context 🔦

Just trying to have a full width feature to work properly. I did try to change the paperFullWidth: property, but that ends up changing the root element.

Your Environment 🌎

Tech	Version
Material-UI	v4.1.3
React	16.8.6
Browser	Firefox development latest
TypeScript
etc.

[support]

👋 Thanks for using Material-UI!

We use the issue tracker exclusively for bug reports and feature requests, however,
this issue appears to be a support request or question. Please ask on StackOverflow where the
community will do their best to help. There is a "material-ui" tag that you can use to tag your
question.

If you would like to link from here to your question on SO, it will help others find it.
If your issues is confirmed as a bug, you are welcome to reopen the issue using the issue template.

[joshwooding]

maxWidth is a prop on the Dialog component (https://material-ui.com/api/dialog/#props).

You might be looking for fullScreen?

[BernardA]

I am aware maxWidth is a prop. What I meant is that if you look at fullWidth Description on the same page you indicated above, If true, the dialog stretches to maxWidth.

So it should stretch to maxWidth which defaults to sm. And no, I am not looking for fullScreen.

If you say it's fullWidth and then you take 48px margin on each side on a 320px wide screen, that's not really fullWidth and it looks poorly.

And yes, I believe this is an actual bug.

[BernardA]

Here as it looks as is: ( as shown on firefox dev tools)

This is what I believe is meant by full width.

[joshwooding]

My bad, I believe this has been brought up before but I forgot what was spoken about cc @oliviertassinari

[oliviertassinari]

@joshwooding The described behavior looks good to me. I would expect CSS overrides can solve the problem.

[BernardA]

So, full width is not actually full width and one has to hack it in order for it to work properly.
If you plan to leave it as is, then can you elaborate on the hack, please?

[oliviertassinari]

On a different topic, I would propose that we reduce the default margin:

diff --git a/packages/material-ui/src/Dialog/Dialog.js b/packages/material-ui/src/Dialog/Dialog.js
index 3b8dbe760..ec46b2333 100644
--- a/packages/material-ui/src/Dialog/Dialog.js
+++ b/packages/material-ui/src/Dialog/Dialog.js
@@ -50,7 +50,7 @@ export const styles = theme => ({
   },
   /* Styles applied to the `Paper` component. */
   paper: {
-    margin: 48,
+    margin: 32,
     position: 'relative',
     overflowY: 'auto', // Fix IE 11 issue, to remove at some point.
     '@media print': {
@@ -62,7 +62,7 @@ export const styles = theme => ({
   paperScrollPaper: {
     display: 'flex',
     flexDirection: 'column',
-    maxHeight: 'calc(100% - 96px)',
+    maxHeight: 'calc(100% - 64px)',
   },
   /* Styles applied to the `Paper` component if `scroll="body"`. */
   paperScrollBody: {
@@ -72,14 +72,14 @@ export const styles = theme => ({
   },
   /* Styles applied to the `Paper` component if `maxWidth=false`. */
   paperWidthFalse: {
-    maxWidth: 'calc(100% - 96px)',
+    maxWidth: 'calc(100% - 64px)',
   },
   /* Styles applied to the `Paper` component if `maxWidth="xs"`. */
   paperWidthXs: {
     maxWidth: Math.max(theme.breakpoints.values.xs, 444),
     '&$paperScrollBody': {
-      [theme.breakpoints.down(Math.max(theme.breakpoints.values.xs, 444) + 48 * 2)]: {
-        maxWidth: 'calc(100% - 96px)',
+      [theme.breakpoints.down(Math.max(theme.breakpoints.values.xs, 444) + 32 * 2)]: {
+        maxWidth: 'calc(100% - 64px)',
       },
     },
   },
@@ -87,8 +87,8 @@ export const styles = theme => ({
   paperWidthSm: {
     maxWidth: theme.breakpoints.values.sm,
     '&$paperScrollBody': {
-      [theme.breakpoints.down(theme.breakpoints.values.sm + 48 * 2)]: {
-        maxWidth: 'calc(100% - 96px)',
+      [theme.breakpoints.down(theme.breakpoints.values.sm + 32 * 2)]: {
+        maxWidth: 'calc(100% - 64px)',
       },
     },
   },
@@ -96,8 +96,8 @@ export const styles = theme => ({
   paperWidthMd: {
     maxWidth: theme.breakpoints.values.md,
     '&$paperScrollBody': {
-      [theme.breakpoints.down(theme.breakpoints.values.md + 48 * 2)]: {
-        maxWidth: 'calc(100% - 96px)',
+      [theme.breakpoints.down(theme.breakpoints.values.md + 32 * 2)]: {
+        maxWidth: 'calc(100% - 64px)',
       },
     },
   },
@@ -105,8 +105,8 @@ export const styles = theme => ({
   paperWidthLg: {
     maxWidth: theme.breakpoints.values.lg,
     '&$paperScrollBody': {
-      [theme.breakpoints.down(theme.breakpoints.values.lg + 48 * 2)]: {
-        maxWidth: 'calc(100% - 96px)',
+      [theme.breakpoints.down(theme.breakpoints.values.lg + 32 * 2)]: {
+        maxWidth: 'calc(100% - 64px)',
       },
     },
   },
@@ -114,14 +114,14 @@ export const styles = theme => ({
   paperWidthXl: {
     maxWidth: theme.breakpoints.values.xl,
     '&$paperScrollBody': {
-      [theme.breakpoints.down(theme.breakpoints.values.xl + 48 * 2)]: {
-        maxWidth: 'calc(100% - 96px)',
+      [theme.breakpoints.down(theme.breakpoints.values.xl + 32 * 2)]: {
+        maxWidth: 'calc(100% - 64px)',
       },
     },
   },
   /* Styles applied to the `Paper` component if `fullWidth={true}`. */
   paperFullWidth: {
-    width: 'calc(100% - 96px)',
+    width: 'calc(100% - 64px)',
   },
   /* Styles applied to the `Paper` component if `fullScreen={true}`. */
   paperFullScreen: {

@BernardA Would it help in your case?

[eps1lon]

If you say it's fullWidth and then you take 48px margin on each side on a 320px wide screen, that's not really fullWidth and it looks poorly.

Kind of depends what the target audience for these props are. In the CSS box model we separate width, padding and margin. So if padding or margin are greater than 0 then the width will not be equal to used horizontal space (i.e. perceived width). So for people not familiar with CSS it will indeed be confusing. Otherwise I'd say the name is fitting.

We can improve documentation though and be explicit about the difference between fullWidth and fullScreen

[BernardA]

@eps1lon Your comment may be fine for the Illuminati, as long as technicality is concerned. Common sense may disagree with that though. But if you look at the 2 pictures I posted above and you still think that your concept of full width is fine, then I rest my case.
@oliviertassinari From what I can see you are just reducing the margin to 32px on each side. From my perspective, again just from the visual aspect of the above pictures, you are moving in the right direction, but it stills looks poorly. It should take 100% of the screen in order to look decent on mobile. If I remember right and correct me if I am wrong , fullScreen works just like that, ie, no margins at all. I see no reason why it should be different for fullWidth

[eps1lon]

But if you look at the 2 pictures I posted above and you still think that your concept of full width is fine, then I rest my case.

As I said it's not my concept but that applied in CSS. I completely understand that it might not be intuitive for people not familiar with CSS which is why I suggested the docs improvement approach.

But since this is concerned with styles, styles are implemented with CSS and by people using CSS fullWidth should be concerned with width from CSS. As far as I know this is the first report confusing width with size. If they get more common we can revisit the issue.

[BernardA]

In that case I'd suggest you change that property from fullWidth to fullScreenWidth. Otherwise it's meaningless, given that you can change the visual aspect, and the common sense understanding, with an arbitrary margin.
I've just tested, and confirmed that for fullScreen, you take the REAL full screen. Following your logic, one could just as well decide to place an arbitrary margin there and say, well it's full screen, except for the margin. One would normally understand full screen as full screen width AND full screen height, as you have actually applied in that case.

[eps1lon]

In that case I'd suggest you change that property from fullWidth to fullScreenWidth

If we ignore margin, yes. The current fullWidth behavior matches the width behavior in the CSS box model.

fullScreen is a shorter alias for fullScreenWidthAndFullScreenHeight

Following your logic, one could just as well decide to place an arbitrary margin there and say, well it's full screen, except for the margin.

Following the logic of the CSS box model you could create a dialog with arbitrary margin and call it full width, yes. It wouldn't be full screen, no. That's why we have the separate prop.

How would you propose fullScreen should behave if fullWidth already uses the full screen?

[BernardA]

I did not propose at all that fullWidth is full screen ( height and width ), but only full screen width. It should be plain to see, pun intended, that if you have a property that is meant to be full width, but then you play around with margins so that it becomes something else completely, you are defeating the purpose.
What purpose does your current fullWidth property serve?
As it is it should be called fullTheoreticalWidthThatYouCanNotReallySeeFullScreenWidthCauseWeDecidedToThrowInSomeArbitraryMargin.

[oliviertassinari]

We have precedents for a disableSpacing prop. We could imagine a disableMargin prop for the dialog 🤔. Unless that's already what fullScreen does?

Ok, aside from the default margin reduction, I would propose that we update the prop description:

diff --git a/packages/material-ui/src/Dialog/Dialog.js b/packages/material-ui/src/Dialog/Dialog.js
index 3b8dbe760..25d30923b 100644
--- a/packages/material-ui/src/Dialog/Dialog.js
+++ b/packages/material-ui/src/Dialog/Dialog.js
@@ -290,6 +290,8 @@ Dialog.propTypes = {
   fullScreen: PropTypes.bool,
   /**
    * If `true`, the dialog stretches to `maxWidth`.
+   *
+   * Notice that the dialog width grow is limited by the default margin.
    */
   fullWidth: PropTypes.bool,
   /**

[m2mathew]

I made a dialog yesterday that looks like this on mobile.

I did it by passing down this prop to the Dialog component:

classes={{ paper: classes.dialogPaper }}

Which is applying this style from jss:

dialogPaper: {
  [theme.breakpoints.down(480)]: {
    margin: 24,
  },
},

It seems in line with what @BernardA is wanting. I am overriding the margin for the dialog on a smaller screen (he would want margin: 0, seemingly) and also using a maxWidth of xs for the dialog.

From the MUI Docs

fullWidth will cause the content inside the dialog to stretch to meet one of the enumerated maxWidth values ("xs", "sm", et al.). I find no confusion with the way that this API is named considering it matches the way that CSS works since those enumerations are stand-ins for real width values (300px, 440px, et all).

That being said, perhaps @oliviertassinari's proposed wording update would help, too.

[oliviertassinari]

@m2mathew Thanks for confirming it. So the prop description update is the preferred resolution path for this issue :).

[jahooma]

For our app, we found 48px margin on dialogs to be too much on the smallest screens. We added media queries to change it to 40px for max-width: 375px and 32px for even smaller.

I think something like this should be the default. This caused much trouble for us as, for example, our login buttons were too wide to fit in a dialog on a screen of size 320px because of the large margin.

[oliviertassinari]

@jahooma I think that we should reduce the margin on mobile devices, I agree. Do you want to work on a pull request? We could start from #16560 (comment) :)

[vipertechofficial]

dialog: {
        [theme.breakpoints.down('sm')]: {
            "& .MuiDialog-container .MuiDialog-paper": {
                margin: "24px 0px",
                borderRadius: 0
            },
        }
    }

[Antibreez]

dialog: {
        [theme.breakpoints.down('sm')]: {
            "& .MuiDialog-container .MuiDialog-paper": {
                margin: "24px 0px",
                borderRadius: 0
            },
        }
    }

Can you plaese tell me where should I put this code?

[vipertechofficial]

@Antibreez Look at this example: https://github.com/crypto-red/crypto-red.github.io/blob/master/src/js/components/AccountDialogCreate.js#L648

[Antibreez]

@Antibreez Look at this example: https://github.com/crypto-red/crypto-red.github.io/blob/master/src/js/components/AccountDialogCreate.js#L648

Thank you!