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

[TileMap][RegionMap] Implement custom renderers and expression builders #84775

Merged
merged 45 commits into from
Jan 14, 2021

Conversation

sulemanof
Copy link
Contributor

@sulemanof sulemanof commented Dec 2, 2020

Summary

Part of #46801

Implement custom renderers and expression builders for the Tile Map & Region Map visualizations.

The PR includes next changes:

Tile Map plugin

  • vis expression builder - src/plugins/tile_map/public/to_ast.ts;
  • vis renderer - src/plugins/tile_map/public/tile_map_renderer.tsx;
  • lazy load TileMapOptions component for editor;
  • couple of functions. such as decodeGeoHash, geoContains, convertToGeoJson were moved directly into tile_map plugin, since they are only used there;
  • the touched files were converted into typescript;
  • removed unused styles src/plugins/tile_map/public/_tile_map.scss;

Region Map plugin

  • vis expression builder - src/plugins/region_map/public/to_ast.ts;
  • vis renderer - src/plugins/region_map/public/region_map_renderer.tsx;
  • lazy load RegionMapOptions component for editor;
  • the touched files were converted into typescript;

Maps legacy plugin

  • adjust BaseMapsVisualization parent class to use IInterpreterRenderHandlers and VisParams instead of ExprVis instance - which will be removed in visualizations cleanup;
  • adjust KibanaMap class to use uiState object directly;

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@sulemanof sulemanof mentioned this pull request Dec 17, 2020
13 tasks
@sulemanof sulemanof changed the title [Tilemap] Implement custom renderer [TileMap][RegionMap] Implement custom renderers and expression builders Dec 17, 2020
@botelastic botelastic bot added the Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) label Dec 22, 2020
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.

KibanaApp code review LGTM. I tested locally but not so thoroughly. I can't find any regressions or to be completely honest I found some bugs but exist on master too so weren't created by this PR.

@sulemanof
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@alexwizp alexwizp left a comment

Choose a reason for hiding this comment

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

LGTM, plugins work as expected

src/plugins/maps_legacy/public/map/decode_geo_hash.ts Outdated Show resolved Hide resolved
});
this._leafletMap.on('load', () => {
visualization.sessionState.mapBounds = this.getBounds();
});
this.on('dragend', persistMapStateInUiState);
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these changes being captured somewhere else now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't find any other usages of sessionState except this place, so I think this is just a legacy leftover which should. be removed

src/plugins/tile_map/public/plugin.ts Show resolved Hide resolved
@kindsun
Copy link
Contributor

kindsun commented Dec 29, 2020

Overall some nice clean up, shuffling of existing logic and additions in this PR. Lazy loading is a great add 💯. Posted some minor comments but local tests all appear to work as expected!

image

@stratoula
Copy link
Contributor

@elasticmachine merge upstream

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.

app arch related code LGTM

@thomasneirynck thomasneirynck removed their request for review January 11, 2021 03:37
# Conflicts:
#	src/plugins/tile_map/public/tile_map_type.ts
.ci/Dockerfile Outdated Show resolved Hide resolved
.node-version Outdated Show resolved Hide resolved
@alexwizp
Copy link
Contributor

@aaronjcaldwell @nreese please review that. We need a review from your team

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

LGTM
code review, tested in chrome

src/plugins/tile_map/public/types.ts Outdated Show resolved Hide resolved
src/plugins/tile_map/public/types.ts Outdated Show resolved Hide resolved
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
mapsLegacy 188 183 -5
regionMap 15 30 +15
tileMap 21 39 +18
total +28

Async chunks

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

id before after diff
mapsLegacy 722.0KB 721.7KB -224.0B
regionMap 260.2KB 292.9KB +32.7KB
tileMap 258.5KB 291.7KB +33.2KB
total +65.7KB

Page load bundle

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

id before after diff
expressions 200.6KB 200.6KB +1.0B
mapsLegacy 94.2KB 88.9KB -5.3KB
regionMap 44.4KB 29.3KB -15.1KB
tileMap 44.5KB 27.8KB -16.7KB
visualizations 171.0KB 170.1KB -932.0B
total -38.0KB
Unknown metric groups

async chunk count

id before after diff
regionMap 1 3 +2
tileMap 1 4 +3
total +5

History

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

@sulemanof sulemanof merged commit fbfc509 into elastic:master Jan 14, 2021
@sulemanof sulemanof deleted the feat/tilemap branch January 14, 2021 07:45
sulemanof pushed a commit that referenced this pull request Jan 14, 2021
…rs (#84775) (#88308)

* Convert to typescript

* Move related files directly into plugin

* Implement toExpressionAst

* Remove build_pipeline dedicated fn

* Async import converter

* Create a custom renderer

* Remove ExprVis instance usage in maps visualizations

* Use uiState updates

* Create wrapper component

* Update rendering

* Create region map expression renderer

* Remove resize subscription

* Fix custom visualization expression

* Update interpreter functional tests

* Use types from geojson

Co-authored-by: Alexey Antonov <alexwizp@gmail.com>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Alexey Antonov <alexwizp@gmail.com>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Coordinate Map Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) Feature:Region Map release_note:skip Skip the PR/issue when compiling release notes v7.12.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants