Skip to content

Conversation

@VladLasitsa
Copy link
Contributor

@VladLasitsa VladLasitsa commented Jan 18, 2021

Closes: #78395

Summary

The task was to get rid of the use of mapLegacy (since it will be removed soon) and migrate to the MapBox.
As part of this task the following was done:

Release notes

Leaflet (https://leafletjs.com/) map layer in Vega visualization was replaced to MapBox (https://www.mapbox.com/)

Testing notes

To enable map layer, you should specify type=map in the spec configuration.
If you would like also see Vega layer on the map you can open any vega-map visualization from sample data. For example [Flights] Airport Connections (Hover Over Airport) which you can find if add Sample flight data.

Screens

Before:
vega-map-old

After:
vega-map-new

For maintainers

@alexwizp
Copy link
Contributor

@elasticmachine merge upstream

@elastic elastic deleted a comment from kibanamachine Jan 20, 2021
@alexwizp alexwizp force-pushed the mapbox_vega branch 2 times, most recently from 2f8f04b to f1e02e0 Compare January 21, 2021 10:57
@elastic elastic deleted a comment from kibanamachine Jan 21, 2021
add MapServiceSettings class

some work

add tms_raster_layer

add LayerParameters type

clenup view.ts

some cleeanup

fix grammar

some refactoring and add attribution control

Some refactoring

Add some validation for zoom settings and destroy handler

Some refactoring

some work

fix bundle size

Move getZoomSettings to the separate file

update licence

some work

move logger to createViewConfig

add throttling for updating vega layer
@alexwizp
Copy link
Contributor

@elasticmachine merge upstream

@alexwizp alexwizp changed the title Mapbox vega [Vega] Use mapbox instead of leaflet Jan 22, 2021
@alexwizp
Copy link
Contributor

@elasticmachine merge upstream

@alexwizp alexwizp requested a review from nreese January 25, 2021 10:28
Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

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

nothing really jumping out for me. Works as expected.

Some minor comments for cleanup.

@alexwizp alexwizp marked this pull request as ready for review January 27, 2021 20:17
@alexwizp alexwizp requested a review from a team January 27, 2021 20:17
@alexwizp alexwizp requested a review from a team as a code owner January 27, 2021 20:17
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

Copy link
Contributor

@ryankeairns ryankeairns left a comment

Choose a reason for hiding this comment

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

Approving the SCSS change.
I will defer to @miukimiu for the UX review, if needed.

Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

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

lgtm!

thanks a lot @VladLasitsa

This was an important PR for the Maps-team. It will enable us to:

  • continue to work on the removal of region/coordinate maps
    • to remove leaflet from the Kibana codebase
    • to remove dependency on the raster-image service of EMS
  • support on-prem deployments of EMS for Vega-maps users

💯

@alexwizp
Copy link
Contributor

@elasticmachine merge upstream

@stratoula
Copy link
Contributor

@elasticmachine merge upstream

@stratoula
Copy link
Contributor

Also @VladLasitsa we merged the ts references for vega and it seems that it fails. Can you fix it? cc @alexwizp

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

LGTM, tested locally in chrome, everything seems fine 🙂

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
visTypeVega 93 173 +80

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
visTypeVega 1.5MB 2.5MB ⚠️ +989.7KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
visTypeVega 53.6KB 57.4KB +3.8KB
Unknown metric groups

async chunk count

id before after diff
visTypeVega 7 8 +1

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@VladLasitsa VladLasitsa merged commit e8e8f78 into elastic:master Jan 29, 2021
VladLasitsa added a commit to VladLasitsa/kibana that referenced this pull request Jan 29, 2021
* [WIP][Vega] Use mapbox instead of leaflet elastic#78395

add MapServiceSettings class

some work

add tms_raster_layer

add LayerParameters type

clenup view.ts

some cleeanup

fix grammar

some refactoring and add attribution control

Some refactoring

Add some validation for zoom settings and destroy handler

Some refactoring

some work

fix bundle size

Move getZoomSettings to the separate file

update licence

some work

move logger to createViewConfig

add throttling for updating vega layer

* move EMSClient to a separate bundle

* [unit testing] add tests for validation_helper.ts

* [Bundle optimization] lazy loading of '@elastic/ems-client' only if user open map layer

* [Map] fix cursor: crosshair -> auto

* [unit testing] add tests for tms_raster_layer.test

* [unit testing] add tests for vega_layer.ts

* VSI related code was moved into a separate file / unit tests were added

* Add functional test for vega map

* [unit testing] add tests for map_service_setting.ts

* Add unload in function test and delete some unneeded code from test

* road_map -> road_map_desaturated

* [unit testing] add more tests for map_service_settings.test.ts

* Add unit tests for view.ts

* Fix some remarks

* Fix unit tests

* remove tms_tile_layers enum

* [unit testing] fix map_service_settings.test.ts

* Fix unit test for view.ts

* Fix some comments

* Fix type check

* Fix CI

Co-authored-by: Alexey Antonov <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>
VladLasitsa added a commit that referenced this pull request Feb 1, 2021
* [WIP][Vega] Use mapbox instead of leaflet #78395

add MapServiceSettings class

some work

add tms_raster_layer

add LayerParameters type

clenup view.ts

some cleeanup

fix grammar

some refactoring and add attribution control

Some refactoring

Add some validation for zoom settings and destroy handler

Some refactoring

some work

fix bundle size

Move getZoomSettings to the separate file

update licence

some work

move logger to createViewConfig

add throttling for updating vega layer

* move EMSClient to a separate bundle

* [unit testing] add tests for validation_helper.ts

* [Bundle optimization] lazy loading of '@elastic/ems-client' only if user open map layer

* [Map] fix cursor: crosshair -> auto

* [unit testing] add tests for tms_raster_layer.test

* [unit testing] add tests for vega_layer.ts

* VSI related code was moved into a separate file / unit tests were added

* Add functional test for vega map

* [unit testing] add tests for map_service_setting.ts

* Add unload in function test and delete some unneeded code from test

* road_map -> road_map_desaturated

* [unit testing] add more tests for map_service_settings.test.ts

* Add unit tests for view.ts

* Fix some remarks

* Fix unit tests

* remove tms_tile_layers enum

* [unit testing] fix map_service_settings.test.ts

* Fix unit test for view.ts

* Fix some comments

* Fix type check

* Fix CI

Co-authored-by: Alexey Antonov <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Alexey Antonov <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>
@alexwizp alexwizp mentioned this pull request Apr 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature:Vega Vega visualizations release_note:enhancement Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v7.12.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Vega] Use mapbox instead of leaflet

7 participants