You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "jansule (via GitHub)" <gi...@apache.org> on 2023/11/06 09:56:25 UTC

[PR] feat(plugin): add plugin-chart-cartodiagram [superset]

jansule opened a new pull request, #25869:
URL: https://github.com/apache/superset/pull/25869

   <!---
   Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
   Example:
   fix(dashboard): load charts correctly
   -->
   
   ### SUMMARY
   <!--- Describe the change below, including rationale and design decisions -->
   
   This adds a new plugin `plugin-chart-cartodiagram` that allows creating cartodiagrams, i.e. charts on a map. See also https://github.com/apache/superset/discussions/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](https://openlayers.org/)** as mapping library
   - **[GeoStyler](https://geostyler.org/)** for styling of vector data (disclaimer: I am one of the maintainers of GeoStyler)
   
   Both libraries are part of the [Open Source Geospatial Foundation](https://www.osgeo.org/), 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 https://github.com/apache/superset/pull/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). 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.
   
   ---
   
   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
   <!--- Skip this if not applicable -->
   
   **Example dashboard with some cartodiagrams:**
   
   ![superset-plugin-demo-dashboard](https://github.com/apache/superset/assets/12186477/4f373083-ea87-407b-be0a-f11c464b8073)
   
   **Explore view of cartodiagram plugin (configuration):**
   
   ![superset-plugin-demo-configuration](https://github.com/apache/superset/assets/12186477/829a4b5a-d654-4cda-ba95-af8b0a331671)
   
   **Explore view of cartodiagram plugin (customization):**
   
   ![superset-plugin-demo-customize](https://github.com/apache/superset/assets/12186477/a7056cad-7dba-47c3-a840-796ac147d393)
   
   **Explore view of cartodiagram plugin (layer configuration):**
   
   ![superset-plugin-demo-layer-configuration](https://github.com/apache/superset/assets/12186477/a24909cb-bc8b-4e30-8a8d-0d0c5c7c1fa7)
   
   
   **Explore view of cartodiagram plugin (layer styling):**
   
   ![superset-plugin-demo-layer-styling](https://github.com/apache/superset/assets/12186477/93b7aa9e-d151-43f6-a3b5-afd8dde6e9c3)
   
   ### TESTING INSTRUCTIONS
   <!--- Required! What steps can be taken to manually verify the changes? -->
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [x] Has associated issue: https://github.com/apache/superset/discussions/21758
   - [ ] Required feature flags:
   - [ ] Changes UI
   - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
     - [ ] Migration is atomic, supports rollback & is backwards-compatible
     - [ ] Confirm DB migration upgrade and downgrade tested
     - [ ] Runtime estimates and downtime expectations provided
   - [x] Introduces new feature or API
   - [ ] Removes existing feature or API
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(plugin): add plugin-chart-cartodiagram [superset]

Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #25869:
URL: https://github.com/apache/superset/pull/25869#issuecomment-2369253104

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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(plugin): add plugin-chart-cartodiagram [superset]

Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #25869:
URL: https://github.com/apache/superset/pull/25869#issuecomment-1917396938

   ## [Codecov](https://app.codecov.io/gh/apache/superset/pull/25869?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   All modified and coverable lines are covered by tests :white_check_mark:
   > Comparison is base [(`f31b823`)](https://app.codecov.io/gh/apache/superset/commit/f31b823343efe615b151009160032223adac5982?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 67.24% compared to head [(`dc16320`)](https://app.codecov.io/gh/apache/superset/pull/25869?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 59.64%.
   
   
   <details><summary>Additional details and impacted files</summary>
   
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #25869      +/-   ##
   ==========================================
   - Coverage   67.24%   59.64%   -7.61%     
   ==========================================
     Files        1895     1891       -4     
     Lines       74248    74172      -76     
     Branches     8257     8237      -20     
   ==========================================
   - Hits        49929    44240    -5689     
   - Misses      22248    27868    +5620     
   + Partials     2071     2064       -7     
   ```
   
   | [Flag](https://app.codecov.io/gh/apache/superset/pull/25869/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [hive](https://app.codecov.io/gh/apache/superset/pull/25869/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `53.81% <ø> (?)` | |
   | [mysql](https://app.codecov.io/gh/apache/superset/pull/25869/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [postgres](https://app.codecov.io/gh/apache/superset/pull/25869/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [presto](https://app.codecov.io/gh/apache/superset/pull/25869/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `53.76% <ø> (?)` | |
   | [python](https://app.codecov.io/gh/apache/superset/pull/25869/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `62.51% <ø> (-15.70%)` | :arrow_down: |
   | [sqlite](https://app.codecov.io/gh/apache/superset/pull/25869/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [unit](https://app.codecov.io/gh/apache/superset/pull/25869/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.43% <ø> (?)` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   
   </details>
   
   [:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/25869?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).   
   :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(plugin): add plugin-chart-cartodiagram [superset]

Posted by "jansule (via GitHub)" <gi...@apache.org>.
jansule commented on code in PR #25869:
URL: https://github.com/apache/superset/pull/25869#discussion_r1891553376


##########
superset-frontend/packages/superset-ui-core/src/chart/types/QueryResponse.ts:
##########
@@ -31,6 +31,14 @@ export interface TimeseriesDataRecord extends DataRecord {
   __timestamp: number | string | Date | null;
 }
 
+export const isTimeseriesDataRecord = (
+  item: any,
+): item is TimeseriesDataRecord => Object.keys(item).includes('__timestamp');
+
+export const isTimeseriesDataRecordList = (
+  items: any[],
+): items is TimeseriesDataRecord[] => items.every(isTimeseriesDataRecord);
+

Review Comment:
   Good catch. Removed it.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(plugin): add plugin-chart-cartodiagram [superset]

Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on code in PR #25869:
URL: https://github.com/apache/superset/pull/25869#discussion_r1388486820


##########
superset-frontend/babel.config.js:
##########
@@ -22,6 +22,12 @@ module.exports = {
   sourceMaps: true,
   sourceType: 'module',
   retainLines: true,
+  // 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$/],

Review Comment:
   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.
   
   ```suggestion
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(plugin): add plugin-chart-cartodiagram [superset]

Posted by "lcalisto (via GitHub)" <gi...@apache.org>.
lcalisto commented on PR #25869:
URL: https://github.com/apache/superset/pull/25869#issuecomment-2348644895

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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(plugin): add plugin-chart-cartodiagram [superset]

Posted by "jansule (via GitHub)" <gi...@apache.org>.
jansule commented on code in PR #25869:
URL: https://github.com/apache/superset/pull/25869#discussion_r1891548683


##########
superset-frontend/plugins/plugin-chart-cartodiagram/src/plugin/controlPanel.ts:
##########
@@ -0,0 +1,203 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { t, validateNonEmpty } from '@superset-ui/core';
+import { ControlPanelConfig } from '@superset-ui/chart-controls';
+import { selectedChartMutator } from '../util/controlPanelUtil';
+
+import { MAX_ZOOM_LEVEL, MIN_ZOOM_LEVEL } from '../util/zoomUtil';
+
+const config: ControlPanelConfig = {
+  /**
+   * The control panel is split into two tabs: "Data" and
+   * "Customize". The controls that define the inputs to
+   * the chart data request, such as columns and metrics, usually
+   * reside within "Data", while controls that affect the visual
+   * appearance or functionality of the chart are under the
+   * "Customize" section.
+   */
+
+  // For control input types, see: superset-frontend/src/explore/components/controls/index.js

Review Comment:
   done



##########
superset-frontend/plugins/plugin-chart-cartodiagram/src/plugin/buildQuery.ts:
##########
@@ -0,0 +1,68 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { QueryFormData, getChartBuildQueryRegistry } from '@superset-ui/core';
+
+/**
+ * The buildQuery function is used to create an instance of QueryContext that's
+ * sent to the chart data endpoint. In addition to containing information of which
+ * datasource to use, it specifies the type (e.g. full payload, samples, query) and
+ * format (e.g. CSV or JSON) of the result and whether or not to force refresh the data from
+ * the datasource as opposed to using a cached copy of the data, if available.
+ *
+ * More importantly though, QueryContext contains a property `queries`, which is an array of
+ * QueryObjects specifying individual data requests to be made. A QueryObject specifies which
+ * columns, metrics and filters, among others, to use during the query. Usually it will be enough
+ * to specify just one query based on the baseQueryObject, but for some more advanced use cases
+ * it is possible to define post processing operations in the QueryObject, or multiple queries
+ * if a viz needs multiple different result sets.
+ */

Review Comment:
   done



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(plugin): add plugin-chart-cartodiagram [superset]

Posted by "jansule (via GitHub)" <gi...@apache.org>.
jansule commented on PR #25869:
URL: https://github.com/apache/superset/pull/25869#issuecomment-2419011942

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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(plugin): add plugin-chart-cartodiagram [superset]

Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro merged PR #25869:
URL: https://github.com/apache/superset/pull/25869


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(plugin): add plugin-chart-cartodiagram [superset]

Posted by "jansule (via GitHub)" <gi...@apache.org>.
jansule commented on PR #25869:
URL: https://github.com/apache/superset/pull/25869#issuecomment-1805288632

   > @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 https://github.com/apache/superset/pull/24272. I guess it just got lost in the amount of PRs you are receiving.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(plugin): add plugin-chart-cartodiagram [superset]

Posted by "jansule (via GitHub)" <gi...@apache.org>.
jansule commented on PR #25869:
URL: https://github.com/apache/superset/pull/25869#issuecomment-1803419549

   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).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(plugin): add plugin-chart-cartodiagram [superset]

Posted by "pesekon2 (via GitHub)" <gi...@apache.org>.
pesekon2 commented on PR #25869:
URL: https://github.com/apache/superset/pull/25869#issuecomment-2363515331

   > 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](https://github.com/user-attachments/assets/cb4d4550-82a9-414f-98a5-9a96c7fb47aa)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(plugin): add plugin-chart-cartodiagram [superset]

Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #25869:
URL: https://github.com/apache/superset/pull/25869#issuecomment-2557558775

   I'm excited we can likely get this into 5.0!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(plugin): add plugin-chart-cartodiagram [superset]

Posted by "jansule (via GitHub)" <gi...@apache.org>.
jansule commented on code in PR #25869:
URL: https://github.com/apache/superset/pull/25869#discussion_r1891548178


##########
superset-frontend/plugins/plugin-chart-echarts/package.json:
##########
@@ -25,13 +25,13 @@
   ],
   "dependencies": {
     "d3-array": "^1.2.0",
-    "echarts": "^5.4.1",
     "lodash": "^4.17.21",
     "moment": "^2.30.1"
   },
   "peerDependencies": {
     "@superset-ui/chart-controls": "*",
     "@superset-ui/core": "*",
+    "echarts": "*",

Review Comment:
   We use it in one of the new controls and thus also need it as a core dependency.



##########
superset-frontend/plugins/plugin-chart-cartodiagram/src/plugin/index.ts:
##########
@@ -0,0 +1,77 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { t, ChartMetadata, ChartPlugin } from '@superset-ui/core';
+import buildQuery from './buildQuery';
+import controlPanel from './controlPanel';
+import transformProps from './transformProps';
+import thumbnail from '../images/thumbnail.png';
+import example1 from '../images/example1.png';
+import example2 from '../images/example2.png';
+import { CartodiagramPluginConstructorOpts } from '../types';
+import { getLayerConfig } from '../util/controlPanelUtil';
+
+export default class CartodiagramPlugin extends ChartPlugin {
+  /**
+   * The constructor is used to pass relevant metadata and callbacks that get
+   * registered in respective registries that are used throughout the library
+   * and application. A more thorough description of each property is given in
+   * the respective imported file.
+   *
+   * It is worth noting that `buildQuery` and is optional, and only needed for
+   * advanced visualizations that require either post processing operations
+   * (pivoting, rolling aggregations, sorting etc) or submitting multiple queries.
+   */

Review Comment:
   done



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(plugin): add plugin-chart-cartodiagram [superset]

Posted by "jansule (via GitHub)" <gi...@apache.org>.
jansule commented on code in PR #25869:
URL: https://github.com/apache/superset/pull/25869#discussion_r1891549736


##########
superset-frontend/plugins/plugin-chart-cartodiagram/src/plugin/transformProps.ts:
##########
@@ -0,0 +1,92 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { ChartProps, getChartTransformPropsRegistry } from '@superset-ui/core';
+import {
+  getChartConfigs,
+  parseSelectedChart,
+} from '../util/transformPropsUtil';
+
+export default function transformProps(chartProps: ChartProps) {
+  /**
+   * This function is called after a successful response has been
+   * received from the chart data endpoint, and is used to transform
+   * the incoming data prior to being sent to the Visualization.
+   *
+   * The transformProps function is also quite useful to return
+   * additional/modified props to your data viz component. The formData
+   * can also be accessed from your CartodiagramPlugin.tsx file, but
+   * doing supplying custom props here is often handy for integrating third
+   * party libraries that rely on specific props.
+   *
+   * A description of properties in `chartProps`:
+   * - `height`, `width`: the height/width of the DOM element in which
+   *   the chart is located
+   * - `formData`: the chart data request payload that was sent to the
+   *   backend.
+   * - `queriesData`: the chart data response payload that was received
+   *   from the backend. Some notable properties of `queriesData`:
+   *   - `data`: an array with data, each row with an object mapping
+   *     the column/alias to its value. Example:
+   *     `[{ col1: 'abc', metric1: 10 }, { col1: 'xyz', metric1: 20 }]`
+   *   - `rowcount`: the number of rows in `data`
+   *   - `query`: the query that was issued.
+   *
+   * Please note: the transformProps function gets cached when the
+   * application loads. When making changes to the `transformProps`
+   * function during development with hot reloading, changes won't
+   * be seen until restarting the development server.
+   */

Review Comment:
   done



##########
superset-frontend/plugins/plugin-chart-cartodiagram/src/CartodiagramPlugin.tsx:
##########
@@ -0,0 +1,59 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { createRef, useState } from 'react';
+import { styled, useTheme } from '@superset-ui/core';
+import OlMap from 'ol/Map';
+import {
+  CartodiagramPluginProps,
+  CartodiagramPluginStylesProps,
+} from './types';
+
+import OlChartMap from './components/OlChartMap';
+
+import 'ol/ol.css';
+
+// The following Styles component is a <div> element, which has been styled using Emotion
+// For docs, visit https://emotion.sh/docs/styled
+
+// Theming variables are provided for your use via a ThemeProvider
+// imported from @superset-ui/core. For variables available, please visit
+// https://github.com/apache-superset/superset-ui/blob/master/packages/superset-ui-core/src/style/index.ts
+
+const Styles = styled.div<CartodiagramPluginStylesProps>`
+  height: ${({ height }) => height}px;
+  width: ${({ width }) => width}px;
+`;
+
+export default function CartodiagramPlugin(props: CartodiagramPluginProps) {
+  const { height, width } = props;
+  const theme = useTheme();
+
+  const rootElem = createRef<HTMLDivElement>();
+
+  const [mapId] = useState(
+    `cartodiagram-plugin-${Math.floor(Math.random() * 1000)}`,
+  );

Review Comment:
   good point. changed.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(plugin): add plugin-chart-cartodiagram [superset]

Posted by "jansule (via GitHub)" <gi...@apache.org>.
jansule commented on PR #25869:
URL: https://github.com/apache/superset/pull/25869#issuecomment-2553324168

   Thanks for the review @villebro! I applied your suggestions. It would be great if you could have another look at it


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(plugin): add plugin-chart-cartodiagram [superset]

Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on PR #25869:
URL: https://github.com/apache/superset/pull/25869#issuecomment-2555256014

   > The frontend-build fails due to `@types/geojson` not being listed as a dependency of `plugins/legacy-preset-chart-deckgl`. Is this something that should be addressed within this PR?
   
   @jansule hmm, this is curious - let's satisfy the linter and see if it all makes sense when CI is green.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(plugin): add plugin-chart-cartodiagram [superset]

Posted by "jansule (via GitHub)" <gi...@apache.org>.
jansule commented on code in PR #25869:
URL: https://github.com/apache/superset/pull/25869#discussion_r1891547254


##########
superset-frontend/plugins/plugin-chart-table/src/utils/extent.ts:
##########
@@ -16,7 +16,6 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-

Review Comment:
   Good catch. Removed changes



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(plugin): add plugin-chart-cartodiagram [superset]

Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #25869:
URL: https://github.com/apache/superset/pull/25869#issuecomment-2375610865

   > @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 https://github.com/bramstein/css-font-parser/pull/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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(plugin): add plugin-chart-cartodiagram [superset]

Posted by "pesekon2 (via GitHub)" <gi...@apache.org>.
pesekon2 commented on PR #25869:
URL: https://github.com/apache/superset/pull/25869#issuecomment-2371588975

   > > 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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(plugin): add plugin-chart-cartodiagram [superset]

Posted by "jansule (via GitHub)" <gi...@apache.org>.
jansule commented on PR #25869:
URL: https://github.com/apache/superset/pull/25869#issuecomment-2553379482

   The frontend-build fails due to `@types/geojson` not being listed as a dependency of `plugins/legacy-preset-chart-deckgl`. Is this something that should be addressed within this PR?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(plugin): add plugin-chart-cartodiagram [superset]

Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #25869:
URL: https://github.com/apache/superset/pull/25869#discussion_r1866577704


##########
superset-frontend/package.json:
##########
@@ -137,11 +138,17 @@
     "d3-scale": "^2.1.2",
     "dayjs": "^1.11.13",
     "dom-to-image-more": "^3.2.0",
+    "echarts": "^5.4.1",
     "emotion-rgba": "0.0.12",
     "fast-glob": "^3.3.2",
     "fs-extra": "^11.2.0",
     "fuse.js": "^7.0.0",
     "geolib": "^2.0.24",
+    "geostyler": "^12.0.2",
+    "geostyler-data": "^1.0.0",
+    "geostyler-openlayers-parser": "^4.3.0",
+    "geostyler-style": "^7.5.0",
+    "geostyler-wfs-parser": "^2.0.3",

Review Comment:
   I agree with @justinpark that these deps (and the `ol` package below) should ideally exist in the plugin. However, I'm aware that due to limitations in the current plugin architecture, we aren't able to adequately decouple controls from the core codebase. This is something that will be considered when redesigning the plugin architecture.



##########
superset-frontend/packages/superset-ui-core/src/chart/types/QueryResponse.ts:
##########
@@ -31,6 +31,14 @@ export interface TimeseriesDataRecord extends DataRecord {
   __timestamp: number | string | Date | null;
 }
 
+export const isTimeseriesDataRecord = (
+  item: any,
+): item is TimeseriesDataRecord => Object.keys(item).includes('__timestamp');

Review Comment:
   nit: there is `DTTM_ALIAS` that should be referenced when referring to `__timestamp`.



##########
superset-frontend/plugins/plugin-chart-cartodiagram/package.json:
##########
@@ -0,0 +1,51 @@
+{
+  "name": "@superset-ui/plugin-chart-cartodiagram",
+  "version": "0.0.1",
+  "description": "An OpenLayers map that displays charts for single features.",
+  "sideEffects": false,
+  "main": "lib/index.js",
+  "module": "esm/index.js",
+  "files": [
+    "esm",
+    "lib"
+  ],
+  "repository": {
+    "type": "git",
+    "url": "git+https://github.com/apache-superset/superset-ui.git"
+  },
+  "keywords": [
+    "superset"
+  ],
+  "license": "Apache-2.0",
+  "bugs": {
+    "url": "https://github.com/apache-superset/superset-ui/issues"
+  },
+  "homepage": "https://github.com/apache-superset/superset-ui#readme",
+  "contributors": [
+    "terrestris GmbH & Co. KG <in...@terrestris.de> (https://www.terrestris.de)",
+    "meggsimum - Büro für Geoinformatik <in...@meggsimum.de> (https://meggsimum.de)"
+  ],
+  "publishConfig": {
+    "access": "public"
+  },
+  "dependencies": {
+    "@types/geojson": "^7946.0.10",
+    "geojson": "^0.5.0",
+    "lodash": "^4.17.21",
+    "ol": "^7.1.0"

Review Comment:
   I see we are already adding `ol` in the main `package.json`. Therefore, should this not be in `peerDependencies`? Or is there some reason we specifically need `^7.1.0` here? It'd be nice if we can reuse the main dependency here, too, as in the future we want to decouple these deps from the core package to the plugins.



##########
superset-frontend/packages/superset-ui-core/src/chart/types/QueryResponse.ts:
##########
@@ -31,6 +31,14 @@ export interface TimeseriesDataRecord extends DataRecord {
   __timestamp: number | string | Date | null;
 }
 
+export const isTimeseriesDataRecord = (
+  item: any,
+): item is TimeseriesDataRecord => Object.keys(item).includes('__timestamp');
+
+export const isTimeseriesDataRecordList = (
+  items: any[],
+): items is TimeseriesDataRecord[] => items.every(isTimeseriesDataRecord);
+

Review Comment:
   Are these still needed? I checked out the codebase, but I wasn't able to find any references to these functions. Maybe they were needed before? The reason I ask is because we're slowly deprecating the concept of dedicated Timeseries charts (most charts should just support any type of base axis).



##########
superset-frontend/plugins/plugin-chart-cartodiagram/src/plugin/buildQuery.ts:
##########
@@ -0,0 +1,68 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { QueryFormData, getChartBuildQueryRegistry } from '@superset-ui/core';
+
+/**
+ * The buildQuery function is used to create an instance of QueryContext that's
+ * sent to the chart data endpoint. In addition to containing information of which
+ * datasource to use, it specifies the type (e.g. full payload, samples, query) and
+ * format (e.g. CSV or JSON) of the result and whether or not to force refresh the data from
+ * the datasource as opposed to using a cached copy of the data, if available.
+ *
+ * More importantly though, QueryContext contains a property `queries`, which is an array of
+ * QueryObjects specifying individual data requests to be made. A QueryObject specifies which
+ * columns, metrics and filters, among others, to use during the query. Usually it will be enough
+ * to specify just one query based on the baseQueryObject, but for some more advanced use cases
+ * it is possible to define post processing operations in the QueryObject, or multiple queries
+ * if a viz needs multiple different result sets.
+ */

Review Comment:
   ```suggestion
   ```



##########
superset-frontend/plugins/plugin-chart-cartodiagram/src/plugin/transformProps.ts:
##########
@@ -0,0 +1,92 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { ChartProps, getChartTransformPropsRegistry } from '@superset-ui/core';
+import {
+  getChartConfigs,
+  parseSelectedChart,
+} from '../util/transformPropsUtil';
+
+export default function transformProps(chartProps: ChartProps) {
+  /**
+   * This function is called after a successful response has been
+   * received from the chart data endpoint, and is used to transform
+   * the incoming data prior to being sent to the Visualization.
+   *
+   * The transformProps function is also quite useful to return
+   * additional/modified props to your data viz component. The formData
+   * can also be accessed from your CartodiagramPlugin.tsx file, but
+   * doing supplying custom props here is often handy for integrating third
+   * party libraries that rely on specific props.
+   *
+   * A description of properties in `chartProps`:
+   * - `height`, `width`: the height/width of the DOM element in which
+   *   the chart is located
+   * - `formData`: the chart data request payload that was sent to the
+   *   backend.
+   * - `queriesData`: the chart data response payload that was received
+   *   from the backend. Some notable properties of `queriesData`:
+   *   - `data`: an array with data, each row with an object mapping
+   *     the column/alias to its value. Example:
+   *     `[{ col1: 'abc', metric1: 10 }, { col1: 'xyz', metric1: 20 }]`
+   *   - `rowcount`: the number of rows in `data`
+   *   - `query`: the query that was issued.
+   *
+   * Please note: the transformProps function gets cached when the
+   * application loads. When making changes to the `transformProps`
+   * function during development with hot reloading, changes won't
+   * be seen until restarting the development server.
+   */

Review Comment:
   Let's remove the boilerplate comments
   ```suggestion
   ```



##########
superset-frontend/plugins/plugin-chart-table/src/utils/extent.ts:
##########
@@ -16,7 +16,6 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-

Review Comment:
   Nit: I assume this is a mistake - let's avoid touching unrelated files.



##########
superset-frontend/plugins/plugin-chart-cartodiagram/src/CartodiagramPlugin.tsx:
##########
@@ -0,0 +1,59 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { createRef, useState } from 'react';
+import { styled, useTheme } from '@superset-ui/core';
+import OlMap from 'ol/Map';
+import {
+  CartodiagramPluginProps,
+  CartodiagramPluginStylesProps,
+} from './types';
+
+import OlChartMap from './components/OlChartMap';
+
+import 'ol/ol.css';
+
+// The following Styles component is a <div> element, which has been styled using Emotion
+// For docs, visit https://emotion.sh/docs/styled
+
+// Theming variables are provided for your use via a ThemeProvider
+// imported from @superset-ui/core. For variables available, please visit
+// https://github.com/apache-superset/superset-ui/blob/master/packages/superset-ui-core/src/style/index.ts
+
+const Styles = styled.div<CartodiagramPluginStylesProps>`
+  height: ${({ height }) => height}px;
+  width: ${({ width }) => width}px;
+`;
+
+export default function CartodiagramPlugin(props: CartodiagramPluginProps) {
+  const { height, width } = props;
+  const theme = useTheme();
+
+  const rootElem = createRef<HTMLDivElement>();
+
+  const [mapId] = useState(
+    `cartodiagram-plugin-${Math.floor(Math.random() * 1000)}`,
+  );

Review Comment:
   nit: if it's all the same, maybe throw a few extra zeros here to minimize the risk of a collision (I can only assume suffixing it with 0-999999 won't cause issues vs 0-999)



##########
superset-frontend/plugins/plugin-chart-echarts/package.json:
##########
@@ -25,13 +25,13 @@
   ],
   "dependencies": {
     "d3-array": "^1.2.0",
-    "echarts": "^5.4.1",
     "lodash": "^4.17.21",
     "moment": "^2.30.1"
   },
   "peerDependencies": {
     "@superset-ui/chart-controls": "*",
     "@superset-ui/core": "*",
+    "echarts": "*",

Review Comment:
   I vaguely remember there having been a discussion about why this was needed as a core dependency. Is this really necessary, or can we leave `echarts` as a dependency only on the ECharts plugin?



##########
superset-frontend/plugins/plugin-chart-cartodiagram/src/plugin/controlPanel.ts:
##########
@@ -0,0 +1,203 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { t, validateNonEmpty } from '@superset-ui/core';
+import { ControlPanelConfig } from '@superset-ui/chart-controls';
+import { selectedChartMutator } from '../util/controlPanelUtil';
+
+import { MAX_ZOOM_LEVEL, MIN_ZOOM_LEVEL } from '../util/zoomUtil';
+
+const config: ControlPanelConfig = {
+  /**
+   * The control panel is split into two tabs: "Data" and
+   * "Customize". The controls that define the inputs to
+   * the chart data request, such as columns and metrics, usually
+   * reside within "Data", while controls that affect the visual
+   * appearance or functionality of the chart are under the
+   * "Customize" section.
+   */
+
+  // For control input types, see: superset-frontend/src/explore/components/controls/index.js

Review Comment:
   ```suggestion
   ```



##########
superset-frontend/plugins/plugin-chart-cartodiagram/src/plugin/index.ts:
##########
@@ -0,0 +1,77 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { t, ChartMetadata, ChartPlugin } from '@superset-ui/core';
+import buildQuery from './buildQuery';
+import controlPanel from './controlPanel';
+import transformProps from './transformProps';
+import thumbnail from '../images/thumbnail.png';
+import example1 from '../images/example1.png';
+import example2 from '../images/example2.png';
+import { CartodiagramPluginConstructorOpts } from '../types';
+import { getLayerConfig } from '../util/controlPanelUtil';
+
+export default class CartodiagramPlugin extends ChartPlugin {
+  /**
+   * The constructor is used to pass relevant metadata and callbacks that get
+   * registered in respective registries that are used throughout the library
+   * and application. A more thorough description of each property is given in
+   * the respective imported file.
+   *
+   * It is worth noting that `buildQuery` and is optional, and only needed for
+   * advanced visualizations that require either post processing operations
+   * (pivoting, rolling aggregations, sorting etc) or submitting multiple queries.
+   */

Review Comment:
   ```suggestion
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(plugin): add plugin-chart-cartodiagram [superset]

Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on code in PR #25869:
URL: https://github.com/apache/superset/pull/25869#discussion_r1388481343


##########
superset-frontend/package.json:
##########
@@ -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",
     "emotion-rgba": "0.0.9",
     "fast-glob": "^3.2.7",
     "fontsource-fira-code": "^4.0.0",
     "fs-extra": "^10.0.0",
     "fuse.js": "^6.4.6",
     "geolib": "^2.0.24",
+    "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",

Review Comment:
   Please add these libraries to [your plugin directory](https://github.com/apache/superset/pull/25869/files#diff-2a826b513161e937d6993e06b6525786055cf86cc4e43408b7170a5c58a3beef), not to the root directory. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(plugin): add plugin-chart-cartodiagram [superset]

Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on PR #25869:
URL: https://github.com/apache/superset/pull/25869#issuecomment-1804443784

   @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?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(plugin): add plugin-chart-cartodiagram [superset]

Posted by "jansule (via GitHub)" <gi...@apache.org>.
jansule commented on PR #25869:
URL: https://github.com/apache/superset/pull/25869#issuecomment-2556434510

   > @jansule hmm, this is curious - let's satisfy the linter and see if it all makes sense when CI is green.
   
   Done. Looks like the dependency was actually missing. I'm wondering why this check did not fail before.
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(plugin): add plugin-chart-cartodiagram [superset]

Posted by "jansule (via GitHub)" <gi...@apache.org>.
jansule commented on PR #25869:
URL: https://github.com/apache/superset/pull/25869#issuecomment-2606487029

   Thanks for the analysis @kgabryje. I will take a close look at that


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(plugin): add plugin-chart-cartodiagram [superset]

Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #25869:
URL: https://github.com/apache/superset/pull/25869#issuecomment-2606575201

   Awesome, thank you!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(plugin): add plugin-chart-cartodiagram [superset]

Posted by "jansule (via GitHub)" <gi...@apache.org>.
jansule commented on PR #25869:
URL: https://github.com/apache/superset/pull/25869#issuecomment-2370391195

   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 https://github.com/bramstein/css-font-parser/pull/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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(plugin): add plugin-chart-cartodiagram [superset]

Posted by "pesekon2 (via GitHub)" <gi...@apache.org>.
pesekon2 commented on PR #25869:
URL: https://github.com/apache/superset/pull/25869#issuecomment-2356680616

   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. An item in the geometry column looks the following way: `{'type': 'Point', 'coordinates': [-4.908446676, 48.373637064]}`. However, I get the following error:
   
   ```
   Unexpected error
   
   Error: could not identify an equality operator for type json
   LINE 31:   coords3,
              ^
   ```
   
   Is there an issue in my data or could it be in the code? 
   
   ![image](https://github.com/user-attachments/assets/e06bea39-5655-4cab-85f2-81a568e06af9)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(plugin): add plugin-chart-cartodiagram [superset]

Posted by "lcalisto (via GitHub)" <gi...@apache.org>.
lcalisto commented on PR #25869:
URL: https://github.com/apache/superset/pull/25869#issuecomment-2376613677

   I just saw that @jansule PR https://github.com/bramstein/css-font-parser/pull/11 has been merged. Perhaps that also helps. 🙂


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(plugin): add plugin-chart-cartodiagram [superset]

Posted by "jansule (via GitHub)" <gi...@apache.org>.
jansule commented on code in PR #25869:
URL: https://github.com/apache/superset/pull/25869#discussion_r1891552459


##########
superset-frontend/plugins/plugin-chart-cartodiagram/package.json:
##########
@@ -0,0 +1,51 @@
+{
+  "name": "@superset-ui/plugin-chart-cartodiagram",
+  "version": "0.0.1",
+  "description": "An OpenLayers map that displays charts for single features.",
+  "sideEffects": false,
+  "main": "lib/index.js",
+  "module": "esm/index.js",
+  "files": [
+    "esm",
+    "lib"
+  ],
+  "repository": {
+    "type": "git",
+    "url": "git+https://github.com/apache-superset/superset-ui.git"
+  },
+  "keywords": [
+    "superset"
+  ],
+  "license": "Apache-2.0",
+  "bugs": {
+    "url": "https://github.com/apache-superset/superset-ui/issues"
+  },
+  "homepage": "https://github.com/apache-superset/superset-ui#readme",
+  "contributors": [
+    "terrestris GmbH & Co. KG <in...@terrestris.de> (https://www.terrestris.de)",
+    "meggsimum - Büro für Geoinformatik <in...@meggsimum.de> (https://meggsimum.de)"
+  ],
+  "publishConfig": {
+    "access": "public"
+  },
+  "dependencies": {
+    "@types/geojson": "^7946.0.10",
+    "geojson": "^0.5.0",
+    "lodash": "^4.17.21",
+    "ol": "^7.1.0"

Review Comment:
   Good catch. Changed it.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(plugin): add plugin-chart-cartodiagram [superset]

Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #25869:
URL: https://github.com/apache/superset/pull/25869#issuecomment-2586857645

   Hi @jansule!
   I noticed that this PR has added a huge chunk to the bundle (2.14MB parsed, visible in the top left corner in the first screenshot). For some reason it contains all antd icons (700kB) and pages/Chart module (500kB), which makes this chunk the largest one in the bundle.
   For comparison, second screenshot shows the bundle analysis after reverting this PR.
   It probably won't affect the loading times of the dashboards which don't contain the Cartodiagram chart, but we might want to solve this issue for the sake of loading performance of dashboards that use this plugin (especially for users with slower Internet)
   
   <img width="1728" alt="image" src="https://github.com/user-attachments/assets/0ee38055-6930-4484-9700-422686edbf5b" />
   
   <img width="1728" alt="image" src="https://github.com/user-attachments/assets/68e93eee-9e11-4a44-acd6-c1d6fa0b7e20" />
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(plugin): add plugin-chart-cartodiagram [superset]

Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on code in PR #25869:
URL: https://github.com/apache/superset/pull/25869#discussion_r1388484352


##########
superset-frontend/package.json:
##########
@@ -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",

Review Comment:
   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.
   ```suggestion
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(plugin): add plugin-chart-cartodiagram [superset]

Posted by "jansule (via GitHub)" <gi...@apache.org>.
jansule commented on code in PR #25869:
URL: https://github.com/apache/superset/pull/25869#discussion_r1389083065


##########
superset-frontend/package.json:
##########
@@ -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",
     "emotion-rgba": "0.0.9",
     "fast-glob": "^3.2.7",
     "fontsource-fira-code": "^4.0.0",
     "fs-extra": "^10.0.0",
     "fuse.js": "^6.4.6",
     "geolib": "^2.0.24",
+    "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",

Review Comment:
   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.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org