[Dialog] Improve scroll=body CSS logic #15896
Merged
+26
−17
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Details of bundle changes.Comparing: 831c6cc...5da996d
|
oliviertassinari
added
the
component: dialog
|
@DominikSerafin Thank you for sharing this! If you don't mind, I would like to explore the simplifications potential leveraging your vertical-align method. Can you spot any problem with: diff --git a/packages/material-ui/src/Dialog/Dialog.js b/packages/material-ui/src/Dialog/Dialog.js
index 6ddadabaf..bab4ed461 100644
--- a/packages/material-ui/src/Dialog/Dialog.js
+++ b/packages/material-ui/src/Dialog/Dialog.js
@@ -29,7 +29,6 @@ export const styles = theme => ({
scrollBody: {
overflowY: 'auto',
overflowX: 'hidden',
- width: '100%',
textAlign: 'center',
'&:after': {
content: '""',
@@ -50,8 +49,6 @@ export const styles = theme => ({
},
/* Styles applied to the `Paper` component. */
paper: {
- display: 'flex',
- flexDirection: 'column',
margin: 48,
position: 'relative',
overflowY: 'auto', // Fix IE 11 issue, to remove at some point.
@@ -62,81 +59,44 @@ export const styles = theme => ({
},
/* Styles applied to the `Paper` component if `scroll="paper"`. */
paperScrollPaper: {
+ display: 'flex',
- flex: '0 1 auto',
+ flexDirection: 'column',
maxHeight: 'calc(100% - 96px)',
},
/* Styles applied to the `Paper` component if `scroll="body"`. */
paperScrollBody: {
- margin: '48px auto',
display: 'inline-block',
verticalAlign: 'middle',
textAlign: 'left', // 'initial' doesn't work on IE 11
},
/* Styles applied to the `Paper` component if `maxWidth=false`. */
paperWidthFalse: {
- '&$paperScrollBody': {
- margin: 48,
- maxWidth: 'calc(100% - 96px)',
- },
+ maxWidth: 'calc(100% - 96px)',
},
/* Styles applied to the `Paper` component if `maxWidth="xs"`. */
paperWidthXs: {
maxWidth: Math.max(theme.breakpoints.values.xs, 444),
- '&$paperScrollBody': {
- maxWidth: `calc(${Math.max(theme.breakpoints.values.xs, 444)}px - 96px)`,
- [theme.breakpoints.down(Math.max(theme.breakpoints.values.xs, 444) + 48 * 2)]: {
- margin: 48,
- maxWidth: 'calc(100% - 96px)',
- },
- },
},
/* Styles applied to the `Paper` component if `maxWidth="sm"`. */
paperWidthSm: {
maxWidth: theme.breakpoints.values.sm,
- '&$paperScrollBody': {
- maxWidth: `calc(${theme.breakpoints.values.sm}px - 96px)`,
- [theme.breakpoints.down(theme.breakpoints.values.sm + 48 * 2)]: {
- margin: 48,
- maxWidth: 'calc(100% - 96px)',
- },
- },
},
/* Styles applied to the `Paper` component if `maxWidth="md"`. */
paperWidthMd: {
maxWidth: theme.breakpoints.values.md,
- '&$paperScrollBody': {
- maxWidth: `calc(${theme.breakpoints.values.md}px - 96px)`,
- [theme.breakpoints.down(theme.breakpoints.values.md + 48 * 2)]: {
- margin: 48,
- maxWidth: 'calc(100% - 96px)',
- },
- },
},
/* Styles applied to the `Paper` component if `maxWidth="lg"`. */
paperWidthLg: {
maxWidth: theme.breakpoints.values.lg,
- '&$paperScrollBody': {
- maxWidth: `calc(${theme.breakpoints.values.lg}px - 96px)`,
- [theme.breakpoints.down(theme.breakpoints.values.lg + 48 * 2)]: {
- margin: 48,
- maxWidth: 'calc(100% - 96px)',
- },
- },
},
/* Styles applied to the `Paper` component if `maxWidth="xl"`. */
paperWidthXl: {
maxWidth: theme.breakpoints.values.xl,
- '&$paperScrollBody': {
- maxWidth: `calc(${theme.breakpoints.values.xl}px - 96px)`,
- [theme.breakpoints.down(theme.breakpoints.values.xl + 48 * 2)]: {
- margin: 48,
- maxWidth: 'calc(100% - 96px)',
- },
- },
},
/* Styles applied to the `Paper` component if `fullWidth={true}`. */
paperFullWidth: {
- width: '100%',
+ width: 'calc(100% - 96px)',
},
/* Styles applied to the `Paper` component if `fullScreen={true}`. */
paperFullScreen: {
@@ -148,8 +108,6 @@ export const styles = theme => ({
borderRadius: 0,
'&$paperScrollBody': {
margin: 0,
- width: '100%',
- maxWidth: '100%',
},
},
});Let us know, thanks! |
oliviertassinari
changed the title
|
Hey @oliviertassinari, this seems to have a problem where the dialog is not properly responsive:
|
|
@DominikSerafin Well spotted! Alright, Let's rollback a bit from my too aggressive proposal. diff --git a/packages/material-ui/src/Dialog/Dialog.js b/packages/material-ui/src/Dialog/Dialog.js
index 6ddadabaf..99b26f214 100644
--- a/packages/material-ui/src/Dialog/Dialog.js
+++ b/packages/material-ui/src/Dialog/Dialog.js
@@ -29,7 +29,6 @@ export const styles = theme => ({
scrollBody: {
overflowY: 'auto',
overflowX: 'hidden',
- width: '100%',
textAlign: 'center',
'&:after': {
content: '""',
@@ -50,8 +49,6 @@ export const styles = theme => ({
},
/* Styles applied to the `Paper` component. */
paper: {
- display: 'flex',
- flexDirection: 'column',
margin: 48,
position: 'relative',
overflowY: 'auto', // Fix IE 11 issue, to remove at some point.
@@ -62,30 +59,25 @@ export const styles = theme => ({
},
/* Styles applied to the `Paper` component if `scroll="paper"`. */
paperScrollPaper: {
- flex: '0 1 auto',
+ display: 'flex',
+ flexDirection: 'column',
maxHeight: 'calc(100% - 96px)',
},
/* Styles applied to the `Paper` component if `scroll="body"`. */
paperScrollBody: {
- margin: '48px auto',
display: 'inline-block',
verticalAlign: 'middle',
textAlign: 'left', // 'initial' doesn't work on IE 11
},
/* Styles applied to the `Paper` component if `maxWidth=false`. */
paperWidthFalse: {
- '&$paperScrollBody': {
- margin: 48,
- maxWidth: 'calc(100% - 96px)',
- },
+ maxWidth: 'calc(100% - 96px)',
},
/* Styles applied to the `Paper` component if `maxWidth="xs"`. */
paperWidthXs: {
maxWidth: Math.max(theme.breakpoints.values.xs, 444),
'&$paperScrollBody': {
- maxWidth: `calc(${Math.max(theme.breakpoints.values.xs, 444)}px - 96px)`,
[theme.breakpoints.down(Math.max(theme.breakpoints.values.xs, 444) + 48 * 2)]: {
- margin: 48,
maxWidth: 'calc(100% - 96px)',
},
},
@@ -94,9 +86,7 @@ export const styles = theme => ({
paperWidthSm: {
maxWidth: theme.breakpoints.values.sm,
'&$paperScrollBody': {
- maxWidth: `calc(${theme.breakpoints.values.sm}px - 96px)`,
[theme.breakpoints.down(theme.breakpoints.values.sm + 48 * 2)]: {
- margin: 48,
maxWidth: 'calc(100% - 96px)',
},
},
@@ -105,9 +95,7 @@ export const styles = theme => ({
paperWidthMd: {
maxWidth: theme.breakpoints.values.md,
'&$paperScrollBody': {
- maxWidth: `calc(${theme.breakpoints.values.md}px - 96px)`,
[theme.breakpoints.down(theme.breakpoints.values.md + 48 * 2)]: {
- margin: 48,
maxWidth: 'calc(100% - 96px)',
},
},
@@ -116,9 +104,7 @@ export const styles = theme => ({
paperWidthLg: {
maxWidth: theme.breakpoints.values.lg,
'&$paperScrollBody': {
- maxWidth: `calc(${theme.breakpoints.values.lg}px - 96px)`,
[theme.breakpoints.down(theme.breakpoints.values.lg + 48 * 2)]: {
- margin: 48,
maxWidth: 'calc(100% - 96px)',
},
},
@@ -127,16 +113,14 @@ export const styles = theme => ({
paperWidthXl: {
maxWidth: theme.breakpoints.values.xl,
'&$paperScrollBody': {
- maxWidth: `calc(${theme.breakpoints.values.xl}px - 96px)`,
[theme.breakpoints.down(theme.breakpoints.values.xl + 48 * 2)]: {
- margin: 48,
maxWidth: 'calc(100% - 96px)',
},
},
},
/* Styles applied to the `Paper` component if `fullWidth={true}`. */
paperFullWidth: {
- width: '100%',
+ width: 'calc(100% - 96px)',
},
/* Styles applied to the `Paper` component if `fullScreen={true}`. */
paperFullScreen: {
@@ -148,7 +132,6 @@ export const styles = theme => ({
borderRadius: 0,
'&$paperScrollBody': {
margin: 0,
- width: '100%',
maxWidth: '100%',
},
}, |
|
@oliviertassinari I've re-tested it again in the same browsers and props combinations and seems to be working nicely should I push the commit with these changes or will you do it? |
|
@DominikSerafin Nice! A new commit would be perfect. I would like to look at the print problem then, see how the changes impact it. |
oliviertassinari
approved these changes
May 28, 2019
|
I was suspecting a regression, but it's was a false alarm. I have added the "good for merge" label back. We can merge it tomorrow if nobody notices an issue with the changes. |
|
@DominikSerafin Thanks |
|
@oliviertassinari awesome to see it merged! Thank you also for you help 😃 |
|
@DominikSerafin how to turn off this behavior? Our dialogs have a dynamic loaded part of an additional content at the end, so when content is loaded dialogs are jumping. Also it's better for user see dialogs in the top of the screen, and not in the center. |
|
@testarossaaaaa I would argue the centered dialogs are generally better (especially when e.g. user has monitor with big resolution or 'portrait' orientation). As for your use case I think you could just show a spinner or some placeholder during the content loading that would make the Dialog take the entire viewport height (e.g. you could use CSS Or you can just overwrite Dialog styles so it's on top - https://material-ui.com/customization/components/. |
|
I agree that centered dialogs are a better default. They are a lot of StackOverflow views on this problem with Bootstrap. Changing the alignment is easy: /* scroll="paper" */
.MuiDialog-scrollPaper {
align-items: flex-start;
}
/* scroll="body" */
.MuiDialog-scrollBody:after {
vertical-align: top;
} |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR tackles two things:
Dialog scroll=body is now always vertically centered instead on top of the page / Dialog with scroll=body is not centered #12130
I found out while working on the centering issue that fullWidth=false was ignored with scroll=body. The content was always in full width (just like with fullWidth=true). This should be now fixed.
I've tested this manually with various props (maxWidth, fullWidth, fullScreen, etc.) on various resolutions and browsers (Windows 10 Chrome, Firefox, Edge, IE 11) and seems to be working OK.
This is my second PR to material-ui and I'm not too familiar with its source so please let me know if I did something improperly and I'll get on fixing that!