Skip to content

[vis/resizeChecker] swap out implemenation with ResizeObserver polyfill - #9439

Merged
spalger merged 1 commit into
elastic:masterfrom
spalger:implement/resize-observer
Mar 3, 2017
Merged

[vis/resizeChecker] swap out implemenation with ResizeObserver polyfill#9439
spalger merged 1 commit into
elastic:masterfrom
spalger:implement/resize-observer

Conversation

@spalger

@spalger spalger commented Dec 10, 2016

Copy link
Copy Markdown
Contributor

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

@spalger

spalger commented Dec 10, 2016

Copy link
Copy Markdown
Contributor Author

jenkins, test this

@ppisljar

ppisljar commented Dec 11, 2016

Copy link
Copy Markdown
Contributor

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

ppisljar commented Dec 11, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

spalger commented Dec 12, 2016

Copy link
Copy Markdown
Contributor Author

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

ppisljar commented Dec 12, 2016

Copy link
Copy Markdown
Contributor

jenkins, test this

@ppisljar

ppisljar commented Dec 12, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

spalger commented Dec 12, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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.

ppisljar
ppisljar approved these changes Dec 12, 2016
View reviewed changes

@ppisljar ppisljar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@thomasneirynck thomasneirynck mentioned this pull request Dec 12, 2016
Closed
@Bargs Bargs mentioned this pull request Dec 12, 2016
Merged
@spalger
spalger force-pushed the implement/resize-observer branch 2 times, most recently from acd3d23 to 495c1e4 Compare December 14, 2016 16:27
@spalger
spalger force-pushed the implement/resize-observer branch from 495c1e4 to e07efde Compare December 14, 2016 16:27
Bargs
Bargs reviewed Dec 15, 2016
View reviewed changes
Comment thread src/ui/public/resize_checker/resize_checker.js
}

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

@Bargs Bargs Dec 15, 2016

Copy link
Copy Markdown
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

@spalger spalger Dec 16, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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)

@spalger spalger Dec 16, 2016

Copy link
Copy Markdown
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.

@Bargs Bargs Dec 16, 2016

Copy link
Copy Markdown
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.

@spalger spalger Dec 16, 2016

Copy link
Copy Markdown
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

stacey-gammon commented Apr 19, 2017

Copy link
Copy Markdown

Any reason this wasn't backported?

@spalger

spalger commented Jun 13, 2017

Copy link
Copy Markdown
Contributor Author

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
patrykkopycinski pushed a commit to patrykkopycinski/kibana that referenced this pull request May 6, 2026
@spalger
[ui/resize_checker] extract from vislib and use resize-observer polyf…
2c1d9e8 
…ill (elastic#9439)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ppisljar ppisljar ppisljar approved these changes
+1 more reviewer
@Bargs Bargs Bargs left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@ppisljar ppisljar

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@spalger @ppisljar @stacey-gammon @Bargs @epixa @thomasneirynck @jimgoodwin