[core][Drawer] Refactor getScrollbarSize usages #43828
Conversation
Netlify deploy previewhttps://deploy-preview-43828--material-ui.netlify.app/ Bundle size reportDetails of bundle changes (Toolpad) |
|
It seems that I'm not able to add a label nor to take a decision for the @argos-ci check, could someone help me out here? |
|
When working on this, I found a little inconsistency with function export default function ownerWindow(node: Node | undefined): Window {
// NOTE: Just a concept, TS doesn't like this!
// Check if node is document element
if (node && node.defaultView) {
return node.defaultView || window;
}
// node is not document
const doc = ownerDocument(node);
return doc.defaultView || window;
}It might not be a big issue, but I found it odd to do:
I might be going through a rabbit hole here, but I think this could do for another refactoring issue. |
Janpot
added
the
component: modal
| @@ -63,7 +63,7 @@ describe('<MenuList />', () => { | |||
| }); | |||
|
|
|||
| describe('actions: adjustStyleForScrollbar', () => { | |||
| const expectedPadding = `${getScrollbarSize(document)}px`; | |||
| const expectedPadding = `${getScrollbarSize(document.defaultView)}px`; | |||
There was a problem hiding this comment.
I believe for those tests, the following could be in order:
| const expectedPadding = `${getScrollbarSize(document.defaultView)}px`; | |
| const expectedPadding = `${getScrollbarSize(window)}px`; |
There was a problem hiding this comment.
Oh, didn't see it at first, but makes sense. It would also remove the need of checking for null in the function. I'll make a commit
| @@ -1,7 +1,10 @@ | |||
| // A change of the browser zoom change the scrollbar size. | |||
| // Credit https://github.com/twbs/bootstrap/blob/488fd8afc535ca3a6ad4dc581f5e89217b6a36ac/js/src/util/scrollbar.js#L14-L18 | |||
| export default function getScrollbarSize(doc: Document): number { | |||
| export default function getScrollbarSize(win?: Window | null): number { | |||
There was a problem hiding this comment.
| export default function getScrollbarSize(win?: Window | null): number { | |
| export default function getScrollbarSize(win: Window = window): number { |
| win = window; | ||
| } | ||
| const documentWidth = win.document.documentElement.clientWidth; | ||
| return Math.abs(win.innerWidth - documentWidth); |
There was a problem hiding this comment.
We could remove the abs here as per #43827 (comment)
| return Math.abs(win.innerWidth - documentWidth); | |
| return win.innerWidth - documentWidth; |
| // https://developer.mozilla.org/en-US/docs/Web/API/Window/innerWidth#usage_notes | ||
| const documentWidth = doc.documentElement.clientWidth; | ||
| return Math.abs(doc.defaultView!.innerWidth - documentWidth); | ||
| if (!win) { |
oliviertassinari
changed the title
Fixes #43827
Refactors all usages of
getScrollbarSizefunction, as mentioned the previous issue.The main change was making the function to use
Windowinstead ofDocument(and also consider using the main window as its default).Then changing every invocation as one of the following:
getScrollbarSize(document)->getScrollbarSize(window).getScrollbarSize(ownerDocument(<NODE>))->getScrollbarSize(ownerWindow(<NODE>)).