Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Dialog with scroll=body is not centered #12130

Closed
Mangatt opened this issue Jul 13, 2018 · 7 comments
Closed

Dialog with scroll=body is not centered #12130

Mangatt opened this issue Jul 13, 2018 · 7 comments
Labels
component: dialog This is the name of the generic UI component, not the React module! out of scope The problem looks valid but we won't fix it (maybe we will revisit it in the future)

Comments

@Mangatt
Copy link
Contributor

Mangatt commented Jul 13, 2018

Expected Behavior

I think that dialog should look the same whether is scroll set to paper or body.

Current Behavior

  • scroll=paper - dialog has auto width and is centered vertically
  • scroll=body - dialog has set width and at top of the screen

Your Environment

Material UI 1.3.1
@oliviertassinari
Copy link
Member

I believe it's a technical limitation. You have to make a tradeoff picking one over the other.

@oliviertassinari oliviertassinari added the component: dialog This is the name of the generic UI component, not the React module! label Jul 13, 2018
@oliviertassinari oliviertassinari added the out of scope The problem looks valid but we won't fix it (maybe we will revisit it in the future) label Jul 21, 2018
@DominikSerafin
Copy link
Contributor

@oliviertassinari if the styling is the only issue here - then I have a solution for that.

It's possible to achieve the desired horizontal and vertical centering while retaining "body" scroll and really good browser compatibility.

The trick here is to use two elements with vertical-align: middle. This quick demo I prepared showcases this a bit better than any explanations:
https://codesandbox.io/s/createreactapp-d1ldz

(I'd try to make a PR myself but I'm a bit scared by the MUI size and it would probably take me a lot of time to do that properly.)

@oliviertassinari
Copy link
Member

The size shouldn't matter :).
I'm skeptical it can work, but I'm would love to be proven wrong! Let me know if you need help.

@DominikSerafin
Copy link
Contributor

DominikSerafin commented May 26, 2019 via email

@oliviertassinari
Copy link
Member

Would it require creating new DOM elements or to introduce a breaking change?

@oliviertassinari
Copy link
Member

@DominikSerafin Aside from the two potential blockers, I can't think of anything else.

@DominikSerafin DominikSerafin mentioned this issue May 27, 2019
Merged
1 task
@DominikSerafin
Copy link
Contributor

DominikSerafin commented May 27, 2019 •
edited
Loading

@oliviertassinari I've opened PR 😃

I was able to achieve this functionality with &:after styling - so no new DOM elements and no breaking changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
component: dialog This is the name of the generic UI component, not the React module! out of scope The problem looks valid but we won't fix it (maybe we will revisit it in the future)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@Mangatt @oliviertassinari @DominikSerafin