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

feat(plugin): add plugin-chart-cartodiagram #25869

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jansule
Copy link
Contributor

@jansule jansule commented Nov 6, 2023

SUMMARY

This adds a new plugin plugin-chart-cartodiagram that allows creating cartodiagrams, i.e. charts on a map. See also #21758.

How does it work?

In order to support as many charts as possible, we created some kind of a meta plugin that takes configurations of any existing chart plugin supported by superset and places it on a map. To do so, we make all charts with a common data source selectable, and inject a group-by property based on a provided geometry column. Then, we use the buildQuery() and transformProps() functions of each chart in combination with the respective chart component (via the ChartComponentRegistry) to render them on a map. Through this, all configuration options of any chart are supported.

Additional configuration options for cartodiagrams are:

  • Map extent: Either fit the view to the extent of the chart data (every item should be visible), or defined a custom extent by moving the map to the desired location.
  • Background layers: Add an arbitrary number of background layers to the map. Currently the formats WMS (raster), XYZ (raster) and WFS (vector) in multiple versions are supported. Vector layers can be styled in the explore view.
  • Background color of chart: The background color and opacity of the chart can be configured in order to improve the readability of a chart depending on the configured background layers.
  • Corner radius of chart: Set the corner radius of the wrapping <div> element of a chart.
  • Zoom based chart size: Set the chart size for each zoom level in order to avoid cluttering. Predefined functions to compute all values are provided (fixed, linear, exponential).

Default layers can be configured in MainPreset.js which will be shown by default for every configured cartodiagram panel. Users are able to edit/remove these layers for a single panel in the explore view. Default layers currently include a WMS showing OpenStreetMap data in grayscale.

Disclaimer: The service providing the OpenStreetMap data is a demo service hosted by terrestris GmbH & Co. KG, which is my employer. On a higher zoomlevel, it shows a watermark for our paid service. The demo server is not recommended to be used in production and we recommend replacing this service. It was just added for others to have a default background map while getting familiar with the plugin and probably should be replaced in the future.

Technical Details/Requirements

Additional libraries:

  • OpenLayers as mapping library
  • GeoStyler for styling of vector data (disclaimer: I am one of the maintainers of GeoStyler)

Both libraries are part of the Open Source Geospatial Foundation, so the chances of future development of those libraries are quite certain.


In order to make use of above libraries, we had to update some dependencies (e.g. typescript, see also #24272), and we also had to make use of npm dedupe in order to resolve version conflicts. Unfortunately, this resulted in a nearly complete rewrite of the package-lock.json.

Due to some version upgrades, we also had to apply some linting fixes, which results in more changed files than actually directly related to this PR. Sorry for that.


The <SelectControl> had to be changed a little to be able to add a customized list of options. We use it to display all charts with a common dataset and also check, if a previously selected chart was updated after the selection.

Limitations

There are some limitations of the plugin, which probably should be addressed in the future:

  • We currently only support geometries in the coordinate reference system (CRS) EPSG:3857 (WGS 84 / Pseudo-Mercator) EPSG:4326 (WGS 84). Additional CRSs might be added in the future.
  • For now, we expect the geometry column to return a GeoJSON point geometry for placing the charts on the map. Additional geometries (e.g. lines and polygons), as well as additional formats (WellKnownText, lat/lon, etc.) might be added in the future.
  • Rendering performance can be improved and varies strongly depending on the used chart type.
  • Currently, filters applied to a dashboard are not reflected in the cartodiagram plugin. We would be grateful, if someone can hint us to the right direction for supporting this. Dashboard filters are now supported.

Since the background layers point to external services, the CSP have to be adjusted. Additional information can be found in the README of the plugin.

How can this be improved?

This PR is just the first step into the direction for additional geospatial support. So there are many things that can be added and improved. Besides the points mentioned above, following points might also be useful:

  • Adding a FeatureFlag to chart plugins that should be allowed to place on a map is probably a nice feature that was also already discussed with @villebro. To do so, the filter options of the REST API need to be extended.
  • Improved caching of the charts in the client, in order to reduce the number of rerenderings of a chart.
  • Adding the geometries of the actual dataset as a background layer and allow styling it.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Example dashboard with some cartodiagrams:

superset-plugin-demo-dashboard

Explore view of cartodiagram plugin (configuration):

superset-plugin-demo-configuration

Explore view of cartodiagram plugin (customization):

superset-plugin-demo-customize

Explore view of cartodiagram plugin (layer configuration):

superset-plugin-demo-layer-configuration

Explore view of cartodiagram plugin (layer styling):

superset-plugin-demo-layer-styling

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue: Generic Map Plugin #21758
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@jansule
Copy link
Contributor Author

jansule commented Nov 9, 2023

I'm not sure why the linting in the Frontend/frontend-build pipeline fails here. On my local setup, I do not get any errors (using node v16.20.2).

@justinpark
Copy link
Member

@jansule Thanks for contributing a new chart plugin. This PR is quite large and includes TypeScript changes. Could you please detach all changes not related to the newly introduced chart plugin?

Comment on lines 149 to 147
"geostyler": "^12.0.2",
"geostyler-data": "^1.0.0",
"geostyler-openlayers-parser": "^4.2.1",
"geostyler-style": "^7.4.0",
"geostyler-wfs-parser": "^2.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

Please add these libraries to your plugin directory, not to the root directory.

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 added these to the root directory as they are needed by the <LayerConfigsControl /> which was added to superset-frontend/src/explore and not to the plugin itself.

The only reason the control is not included in the plugin directory is, that the <ControlFormItem /> was moved from @superset-ui/core to superset-frontend/src/explore, so I cannot import it in the plugin directory. This component, however, is needed for a consistent UI of the controls.

@@ -138,12 +139,18 @@
"d3-scale": "^2.1.2",
"dom-to-image-more": "^2.10.1",
"dom-to-pdf": "^0.3.2",
"echarts": "^5.4.1",
Copy link
Member

Choose a reason for hiding this comment

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

i.e. echarts is included in the plugin-chart-echarts package, and all components that depend on ECharts should also be included in that package.

Suggested change
"echarts": "^5.4.1",

Comment on lines 25 to 30
// After adding transformIgnorePatterns to jest.config.js
// the tests failed. Following line fixed the problem. See also
// https://github.com/babel/babel/issues/8731#issuecomment-423845498
// core-js seems to be packaged as commonjs modules, so it should
// be (more or less) save to ignore them with babel.
ignore: [/^core-js$/],
Copy link
Member

Choose a reason for hiding this comment

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

These are likely side effects of changes to package.json. Please clean up package.json as suggested, and these changes should no longer be necessary.

Suggested change
// After adding transformIgnorePatterns to jest.config.js
// the tests failed. Following line fixed the problem. See also
// https://github.com/babel/babel/issues/8731#issuecomment-423845498
// core-js seems to be packaged as commonjs modules, so it should
// be (more or less) save to ignore them with babel.
ignore: [/^core-js$/],

@jansule
Copy link
Contributor Author

jansule commented Nov 10, 2023

@jansule Thanks for contributing a new chart plugin. This PR is quite large and includes TypeScript changes. Could you please detach all changes not related to the newly introduced chart plugin?

Thanks for the first review @justinpark. I created a PR with only the TypeScript changes a while ago. See #24272. I guess it just got lost in the amount of PRs you are receiving.

@jansule jansule force-pushed the cartodiagram-plugin-2023-11-03 branch from b3c31dc to dc16320 Compare January 30, 2024 16:07
Copy link

codecov bot commented Jan 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2cd7135) 67.17% compared to head (a544528) 69.33%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #25869      +/-   ##
==========================================
+ Coverage   67.17%   69.33%   +2.16%     
==========================================
  Files        1899     1895       -4     
  Lines       74354    74279      -75     
  Branches     8266     8248      -18     
==========================================
+ Hits        49945    51501    +1556     
+ Misses      22360    20735    -1625     
+ Partials     2049     2043       -6     
Flag Coverage Δ
hive 53.80% <ø> (?)
postgres ?
presto 53.75% <ø> (?)
python 82.71% <ø> (+4.52%) ⬆️
sqlite 77.68% <ø> (ø)
unit 56.42% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions github-actions bot added doc Namespace | Anything related to documentation dependencies:npm labels Jan 31, 2024
@jansule jansule force-pushed the cartodiagram-plugin-2023-11-03 branch from 5e554b6 to b317929 Compare January 31, 2024 10:40
@jansule jansule force-pushed the cartodiagram-plugin-2023-11-03 branch from a544528 to 6a774e4 Compare June 27, 2024 14:03
@jansule jansule force-pushed the cartodiagram-plugin-2023-11-03 branch 5 times, most recently from 083b8d7 to 8b59fc8 Compare June 28, 2024 11:20
@jansule
Copy link
Contributor Author

jansule commented Jun 28, 2024

@villebro I finally managed to update the PR so that the pipelines succeed. The failing Dependency Review pipeline fails due to a missing license entry in one of our transitive dependencies (BSD-3 Clause). I added a PR there a while ago that fixes the problem.

So could you take another look at this PR?

@jansule jansule force-pushed the cartodiagram-plugin-2023-11-03 branch from f44ae62 to 1b03c29 Compare June 28, 2024 13:40
@lcalisto
Copy link

Any update on this PR? This would be a very cool feature for Superset.

@pesekon2
Copy link

pesekon2 commented Sep 17, 2024

I would also like to have this PR merged (mapbox is a joke). All my support to @jansule for the patience with updating and conflict solving, etc.

I've tried to test it locally, although not being eligible as a possible reviewer. It seems to work. I am not able to load any WMS as the background but this could possibly be due to some of the proxy/reverseproxy black magic here.

@pesekon2
Copy link

I would also like to have this PR merged (mapbox is a joke). All my support to @jansule for the patience with updating and conflict solving, etc.

I've tried to test it locally, although not being eligible as a possible reviewer. It seems to work. I am not able to load any WMS as the background but this could possibly be due to some of the proxy/reverseproxy black magic here.

Not a proxy magic. Made it work locally with the default terreatris WMS but not being able to change the WMS to anything else. See the example in the screenshot (no WMS visible, no error message in logs; the service URL is https://eagri.cz/public/app/wms/public_zp.fcgi):

image

@rusackas
Copy link
Member

Looks like css-font-parser is making into package-lock.json and has an incompatible license. Any way to get rid of that?

@jansule
Copy link
Contributor Author

jansule commented Sep 24, 2024

Thanks for the feedback! I will try to get some time to resolve the merge conflicts soon.

Looks like css-font-parser is making into package-lock.json and has an incompatible license. Any way to get rid of that?

@rusackas There is an invalid spdx identifier in css-font-parser, but the license itself is compatible (BSD-3-Clause). I created a PR a while ago to solve the issue bramstein/css-font-parser#11 but did not get any feedback.

I am not able to load any WMS as the background but this could possibly be due to some of the proxy/reverseproxy black magic here.

@lcalisto So far I had no problems with loading other WMS. You might want to ensure that your Talisman settings are correct. I will double check as soon as I get time to work on this PR again.

@pesekon2
Copy link

I am not able to load any WMS as the background but this could possibly be due to some of the proxy/reverseproxy black magic here.

@lcalisto So far I had no problems with loading other WMS. You might want to ensure that your Talisman settings are correct. I will double check as soon as I get time to work on this PR again.

Solved it today (I guess it was on me and not on @lcalisto ). It was indeed a problem with Talisman. I didn't make it work with the Talisman so simply turning it off helped but this could very probably be connected to some security issues at the server or some obscure thing about the WMS in use.

Therefore: Big thumbs up. Tested and working for me. Thanks very much and I hope it will get merged one day.

@rusackas
Copy link
Member

@rusackas There is an invalid spdx identifier in css-font-parser, but the license itself is compatible (BSD-3-Clause). I created a PR a while ago to solve the issue bramstein/css-font-parser#11 but did not get any feedback.

Ahh, ok, if it's all good, you can get the Dependency Review CI check to pass by adding the package to allow-dependencies-licenses in the .github/workflows/dependency-review.yml file

@lcalisto
Copy link

I just saw that @jansule PR bramstein/css-font-parser#11 has been merged. Perhaps that also helps. 🙂

@jansule
Copy link
Contributor Author

jansule commented Oct 1, 2024

I just saw that @jansule PR bramstein/css-font-parser#11 has been merged. Perhaps that also helps. 🙂

That's great. I justed pinged the owner to create a new release. Hopefully this will happen soon. Otherwise I will add the dependeny to the allow-dependencies-licenses as @rusackas mentioned.

Solved it today (I guess it was on me and not on @lcalisto ). It was indeed a problem with Talisman. I didn't make it work with the Talisman so simply turning it off helped but this could very probably be connected to some security issues at the server or some obscure thing about the WMS in use.

Happy to hear that it's working for you now!

@jansule jansule force-pushed the cartodiagram-plugin-2023-11-03 branch from 1b03c29 to 51b6e82 Compare October 17, 2024 08:58
@jansule
Copy link
Contributor Author

jansule commented Oct 17, 2024

@rusackas @villebro I just rebased the PR. Could you take another look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies:npm doc Namespace | Anything related to documentation packages plugins size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants