Skip to content
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

Merged
merged 1 commit into from
Mar 3, 2017

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Dec 10, 2016

Discovered the ResizeObserver spec and found a polyfill for it that is far faster than the polling done by the previous implementation

@spalger
Copy link
Contributor Author

spalger commented Dec 10, 2016

jenkins, test this

@ppisljar
Copy link
Member

tests are failing, but it looks its not related to this PR ( broken master ? ):

FAIL: chrome on any platform - kibana - console app - console app - should show the default request (40675ms) expected
'GET _search\n  {\n    "query": { "match_all": {} }\n  }' to equal
'GET _search\n{\n  "query": {\n    "match_all": {}\n  }\n}'

@ppisljar
Copy link
Member

ppisljar commented Dec 11, 2016

funtionally everything seems to work fine

@spalger spalger force-pushed the implement/resize-observer branch 4 times, most recently from 3aed12c to b56c910 Compare December 12, 2016 01:47
@spalger
Copy link
Contributor Author

spalger commented Dec 12, 2016

tests are failing, but it looks its not related to this PR

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.

@ppisljar
Copy link
Member

jenkins, test this

@ppisljar
Copy link
Member

ppisljar commented Dec 12, 2016

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
import {Tooltip, ResizeChecker, Colors} from 'ui/vis/components'

@spalger
Copy link
Contributor Author

spalger commented Dec 12, 2016

I don't feel strongly either way but the resize checker:

  • exports a provider so it would be:

    import {Tooltip, ResizeCheckerProvider, Colors} from 'ui/vis/components'
  • is not only used by visualizations, so console would be importing from ui/vis/components

  • is not a visual component, which seems to conflict with the definition of "components" I'm most familiar with.

Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

}

function getSize(el) {
return [el.clientWidth, el.clientHeight];
Copy link
Contributor

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

Copy link
Contributor Author

@spalger spalger Dec 16, 2016

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)

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 sounds good

@thomasneirynck thomasneirynck removed their request for review January 4, 2017 17:12
@thomasneirynck thomasneirynck removed their assignment Jan 4, 2017
@epixa epixa added v5.3.0 and removed v5.2.0 labels Jan 17, 2017
@epixa epixa removed the v5.3.0 label Feb 10, 2017
@spalger spalger merged commit b804ea0 into elastic:master Mar 3, 2017
@stacey-gammon
Copy link
Contributor

Any reason this wasn't backported?

@spalger
Copy link
Contributor Author

spalger commented Jun 13, 2017

Perhaps just the size and lack of need, is there a reason we should?

@spalger spalger deleted the implement/resize-observer branch June 13, 2017 00:01
@jimgoodwin jimgoodwin added release_note:enhancement Feature:Visualizations Generic visualization features (in case no more specific feature label is available) labels Jul 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Visualizations Generic visualization features (in case no more specific feature label is available) release_note:enhancement review v6.0.0-rc1 v6.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants