Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Maps] Map embeddable #31473
[Maps] Map embeddable #31473
Changes from all commits
9c63c4e
6cf4765
3dd49cf
f59ebeb
1543078
d071b83
d7f5725
b9bdd63
9e0048d
eb1bb15
d192030
5e5d0ae
4faa8d4
2173671
d32242d
5953eeb
2909f76
00eb86f
6b86ce2
3f8e72b
51c0f0a
c717ba3
0bd4a3a
32842b3
423aba6
27575c7
de0e371
1963877
0f934a9
ed3c20e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
nice, this is more clean too.
I don't think we need to wrap this in an animation-frame callback. It assumes we'd get resize events at a rate exceeding 60fps, and that mapbox would not be optimizing for
map.resize()
method calls internally. It'd suprise me if both were true. I think we should be fine just calling resize on each event.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.
@thomasneirynck I have removed the animation frame. Looks like EUI has a ResizeObserver that we could use instead of ui/public/resize_checker. The problem is that ResizeObserver was only added in EUI 7.2.0 and Kibana is currently on EUI 7.1.0. I will make a separate PR to clean this up more and replace resize_checker (which is based on angular) with ResizeObserver once Kibana EUI has been upgraded.
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.
Or, I can just revert the changes to this file and the resize issue can be completely addressed in a separate PR since resize_checker is going to be replaced anyways.
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.
it's good to fix it here imho
OK with new PR where we migrate to eui's ResizeObserver later
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.
There was some reason we needed this previously but I'm comfortable with getting rid of it and letting it re-emerge if necessary. Really might not be needed anymore.
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.
not sure if we should monitor this domNode for a resize (see other comment).