-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[vis/resizeChecker] swap out implemenation with ResizeObserver polyfill #9439
Conversation
jenkins, test this |
tests are failing, but it looks its not related to this PR ( broken master ? ):
|
funtionally everything seems to work fine |
3aed12c
to
b56c910
Compare
The console actually also uses the resize checker, so I moved it out of the vislib and updated the resize checker to support jQuery elements again to fix compatibility. |
jenkins, test this |
what do you think about moving this under ui/vis/components making it clear that its a component visualizations (even 3rd party) should use? (take a look at #9441 ) this way a visualization could then just |
I don't feel strongly either way but the resize checker:
|
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.
LGTM
acd3d23
to
495c1e4
Compare
495c1e4
to
e07efde
Compare
} | ||
|
||
function getSize(el) { | ||
return [el.clientWidth, el.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.
Could this trigger costly layout calculations? Why not use the values provided to the callback? http://rawgit.com/WICG/ResizeObserver/master/index.html#resizeobserverentry
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, can it? I went this route because we need to calculate the size in modifySizeWithoutTriggeringResize()
in order to correctly ignore subsequent resize events, and I didn't want to try to replicate the behavior necessary to calculate the width/height provided by the clientRect (see que-etc/resize-observer-polyfill#4)
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.
Also, it's only called when we are trying to ignore a resize event (on load and in modifySizeWithoutTriggeringResize()
) so I don't think it will be too costly.
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.
Ah, I was thinking it ran on every resize event. Maybe it's not too bad then. It looks like clientWidth
and clientHeight
do force layout. Not sure how else to implement this resize ignoring method though. Maybe it would be worth adding a comment on modifySizeWithoutTriggeringResize
saying "This method can trigger forced layouts, use with caution"?
I think this is a great PR btw! The forced layout thing just jumped out at me because I've been dealing with the same problem elsewhere in the code.
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.
👍 sounds good
Any reason this wasn't backported? |
Perhaps just the size and lack of need, is there a reason we should? |
Discovered the ResizeObserver spec and found a polyfill for it that is far faster than the polling done by the previous implementation