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

Use resizeObserver instead of window resize events, fix and clean tests #2157

Merged
merged 11 commits into from
Feb 10, 2023

Conversation

HarelM
Copy link
Collaborator

@HarelM HarelM commented Feb 9, 2023

This adds the relevant unit test.
Cleans the other tests that has duplicate code in every before function.
Clear some test that did not need some methods.

@rotu Feel free to review :-)

Replaces #2013

@HarelM
Copy link
Collaborator Author

HarelM commented Feb 9, 2023

This is strange... Why would only a small part of the render test fail? Why are they affected from the resize mechanism change?

src/ui/map.ts Show resolved Hide resolved
@HarelM HarelM requested a review from rotu February 10, 2023 07:13
src/ui/map.test.ts Outdated Show resolved Hide resolved
src/ui/map.ts Show resolved Hide resolved
Copy link
Contributor

@wipfli wipfli left a comment

Choose a reason for hiding this comment

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

Looks good to me in general, thanks! I think since the behavior for resizing containers has changes, we should treat this as a breaking change with associated changelog remark. Please also add a test where you only resize the map container but not the window.

@HarelM
Copy link
Collaborator Author

HarelM commented Feb 10, 2023

The unit tests are covering the resize functionality without knowing if the window or the container were resized.
What did you have in mind in terms of testing?

@wipfli
Copy link
Contributor

wipfli commented Feb 10, 2023

Ah OK I thought the test simulate resizing of the window and then check that our code gets called. But if the tests already resizing from container, we do not need more tests.

Copy link
Contributor

@wipfli wipfli left a comment

Choose a reason for hiding this comment

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

Looking forward to use this new behavior. Thanks!

@HarelM
Copy link
Collaborator Author

HarelM commented Feb 10, 2023

I added 2 browser tests, just in case...

nreese added a commit to elastic/kibana that referenced this pull request Jul 13, 2023
maplibre change log
https://github.com/maplibre/maplibre-gl-js/blob/main/CHANGELOG.md#310

Breaking changes that required fixes
* 3.0.0 Remove "mapbox-gl-supported" package from API. If needed, please
reference it directly instead of going through MapLibre.
(maplibre/maplibre-gl-js#2451)
* 3.0.0 Resize map when container element is resized. The
"resize"-related events now has different data associated with it
(maplibre/maplibre-gl-js#2157,
maplibre/maplibre-gl-js#2551). Previously the
originalEvent field was the reason of this change, for example it could
be a resize event from the browser. Now it is ResizeObserverEntry, see
more
[here](https://developer.mozilla.org/en-US/docs/web/api/resizeobserverentry).
* 2.2.0 Improve filter specification typings
(maplibre/maplibre-gl-js#1390)

---------

Co-authored-by: Kibana Machine <[email protected]>
@daylightwarbler daylightwarbler mentioned this pull request Jul 18, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants