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
[core] Remove dom-helpers dependency #14877
Conversation
b25995e
to
15c968f
Compare
const marginRight = parseInt(style.getPropertyValue('margin-right'), 10); | ||
|
||
return marginLeft + doc.body.clientWidth + marginRight < win.innerWidth; | ||
return container.scrollHeight > container.clientHeight; |
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.
@@ -17,7 +17,8 @@ function findIndexOf(data, callback) { | |||
} | |||
|
|||
function getPaddingRight(node) { | |||
return parseInt(css(node, 'paddingRight') || 0, 10); | |||
const win = ownerWindow(node); | |||
return parseInt(win.getComputedStyle(node)['padding-right'] || 0, 10); |
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.
@material-ui/core: parsed: -1.26% 😍, gzip: -1.46% 😍 Details of bundle changes.Comparing: bdb4baa...09babf7
|
I will continue the effort tomorrow, I was interested in the gain potential. It's worth pushing. |
15c968f
to
4d2e6e9
Compare
The biggest win comes from: -import css from 'dom-helpers/style';
-return parseInt(css(node, 'paddingRight') || 0, 10);
+return parseInt(win.getComputedStyle(node)['padding-right'] || 0, 10); |
8380212
to
e7a847d
Compare
e7a847d
to
5bf5528
Compare
container1.style.padding = '20px'; | ||
Object.defineProperty(container1, 'scrollHeight', { | ||
value: 100, | ||
writable: false, |
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.
This will not really prevent writing. All it does is throw an error. I would rather sinon.stub
this with a fake setter:
sinon.stub(container1, 'scrollHeight').get(() => 100).set(() => {})
writable: false
will actually not trigger an error. Would've been nice if sinon would support this out of the box.
@@ -113,15 +122,22 @@ describe('ModalManager', () => { | |||
it('should handle the scroll', () => { | |||
const modal = {}; | |||
const paddingRightBefore = container1.style.paddingRight; | |||
const paddingFixedRightBefore = fixedNode.style.paddingRight; |
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.
Is this just refactoring or did the test actually break?
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.
Both.
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.
So this is a breaking change after all or only breaking in jsdom?
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 think that it's only breaking jsdom.
1138b5f
to
09babf7
Compare
Save -4.92% gzipped on the Modal component.