refactor: set overflow attribute on dialog overlay content change #9903
Conversation
| @@ -68,14 +68,21 @@ export const DialogOverlayMixin = (superClass) => | |||
|
|
|||
| // Update overflow attribute on resize | |||
| this.__resizeObserver = new ResizeObserver(() => { | |||
| this.__updateOverflow(); | |||
| requestAnimationFrame(() => { | |||
There was a problem hiding this comment.
This is needed because otherwise I was getting the following error in the new tests:
Uncaught Error: ResizeObserver loop completed with undelivered notifications.
Using requestAnimationFrame() generally makes sense here (we use setTimeout in ResizeMixin).
| overlay = dialog.$.overlay; | ||
| }); | ||
|
|
||
| describe('slotted content', () => { |
There was a problem hiding this comment.
This is a new test suite that covers changes in the PR.
9a51e62 to
a09f16d
Compare
fe1c823 to
5586810
Compare
|
| @@ -69,26 +69,22 @@ | |||
| height: auto; | |||
| } | |||
|
|
|||
| @media (min-height: 320px) { | |||
There was a problem hiding this comment.
I'm not sure why we had this breakpoint. It seems confusing and overflow attribute would not be set on resizer container scroll, so I changed to align with base styles to only make content part scrollable.
There was a problem hiding this comment.
Yeah, it’s a bit confusing. The idea is that on a very narrow dialog the header and footer would also scroll, to give more space for the content area. But a container query would be the right solution to accomplish that, not a media query, as you could have a narrow dialog even without a narrow viewport.
I think this type of optimization might be better left for application code.
| @@ -69,26 +69,22 @@ | |||
| height: auto; | |||
| } | |||
|
|
|||
| @media (min-height: 320px) { | |||
There was a problem hiding this comment.
Yeah, it’s a bit confusing. The idea is that on a very narrow dialog the header and footer would also scroll, to give more space for the content area. But a container query would be the right solution to accomplish that, not a media query, as you could have a narrow dialog even without a narrow viewport.
I think this type of optimization might be better left for application code.
Description
Updated dialog to always set
overflowattribute even if no header / footer renderer is set.This fixes base styles to make it work e.g. the following case:
Type of change
Note
We could probably debounce calls to
__updateOverflow()but that can be done separately.