-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
refactor(dialog): better handling of scrollable content #2546
Conversation
|
||
// Add flexbox styling if the user is using the `md-dialog-content`. | ||
if ('querySelector' in componentElement) { | ||
this._renderer.setElementClass(componentElement, 'md-dialog-root-flex', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we should do this. This will set display: flex
on the host element of the loaded component, which will potentially change the layout of the contents of that element in a way that the user did not intend; we should generally avoid setting any styles to elements Material doesn't own.
Why not add an element to the dialog-container template for this (and for md-dialog-root
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My reasoning with using flex is that if the user has their content in a md-dialog-content, then it's safe to add flexbox, because they, most likely, don't have any other content at the root (apart from md-dialog-title and md-dialog-actions).
As for adding an extra element, I'm not sure I get what you mean. Isn't this what md-dialog-container already does?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coming back to this PR: which element exactly is this class applied to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I've keeping this around for tracking. I was been working on a different PR that does the something similar, but with a nicer approach. I haven't been able to get it to work properly cross-browser yet. We can close this if necessary.
Currently, the dialog's scrollable content (md-dialog-content) is limited to 65% of the viewport height, however on smaller screens the dialog still ends up being too high. This proposal reworks the `md-dialog-content` to make it take up all of the remaining height, instead of being hardcoded to 65%. The max height is provided by the wrapper instead. Fixes angular#2481. Fixes angular#2775.
8eacecf
to
55f1974
Compare
Hadn't looked at this one in a while and it looks like now there's a possibility for the |
Refactors the focus trap to be used as a directive, rather than a component. This gives us a couple of advantages: * It can be used on the same node as other components. * It removes a level of nesting in the DOM. This makes it slightly more convenient to style projected in cases like the dialog (see angular#2546), where flexbox needs to be applied to the closest possible ancestor. Also includes the following improvements: * No longer triggers change detection when focus hits the start/end anchors. * Resets the anchor tab index when trapping is disabled, instead of removing elements from the DOM. * Adds missing unit tests for the disabled and cleanup logic.
Refactors the focus trap to be used as a directive, rather than a component. This gives us a couple of advantages: * It can be used on the same node as other components. * It removes a level of nesting in the DOM. This makes it slightly more convenient to style projected in cases like the dialog (see angular#2546), where flexbox needs to be applied to the closest possible ancestor. Also includes the following improvements: * No longer triggers change detection when focus hits the start/end anchors. * Resets the anchor tab index when trapping is disabled, instead of removing elements from the DOM. * Adds missing unit tests for the disabled and cleanup logic.
Refactors the focus trap to be used as a directive, rather than a component. This gives us a couple of advantages: * It can be used on the same node as other components. * It removes a level of nesting in the DOM. This makes it slightly more convenient to style projected in cases like the dialog (see angular#2546), where flexbox needs to be applied to the closest possible ancestor. Also includes the following improvements: * No longer triggers change detection when focus hits the start/end anchors. * Resets the anchor tab index when trapping is disabled, instead of removing elements from the DOM. * Adds missing unit tests for the disabled and cleanup logic.
Closing this PR for now; @crisbeto feel free to open a new one |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Currently, the dialog's scrollable content (md-dialog-content) is limited to 65% of the viewport height, however on smaller screens the dialog still ends up being too high. This proposal reworks the
md-dialog-content
to make it take up all of the remaining height, instead of being hardcoded to 65%. The max height is provided by the wrapper instead.Fixes #2481.
Fixes #2775.