refactor(dialog): better handling of scrollable content #2546
Conversation
googlebot
added
the
cla: yes
|
|
||
| // 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.
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.
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.
Coming back to this PR: which element exactly is this class applied to?
There was a problem hiding this comment.
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.
crisbeto
force-pushed
the
2481/dialog-scrolling
branch
from
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-contentto 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.