Retrieve spatialDataColumn in TableWidget for its future usage #900

jmgaya · 2024-09-06T11:12:18Z

Description

Shortcut: https://app.shortcut.com/cartoteam/story/438209/add-support-for-geocolumn-spatialdatacolumn-when-retrieving-table-widget-information?cf_workflow=500091853&ct_workflow=all&vc_group_by=day

Sending this to the API should give us back the information about the spatialDataColumn, which we'll use afterward for zooming into the coordinate

Type of change

Feature

jmgaya · 2024-09-06T11:13:11Z

DEVELOPERS.md

+
+```
+yarn build && ./copy-packages.sh ~/workspace/repositories/cloud-native/workspace-www/node_modules/@carto
+```


@zbigg I've added this, as we both sometimes copy&paste content for the seek of pragmatism when developing things on Carto4React and testing them on cloud-native 😄

jmgaya · 2024-09-06T11:17:53Z

packages/react-api/src/api/model.js

-    queryParams.spatialDataType = spatialDataType;
+    if (spatialFilters) queryParams.spatialFilters = JSON.stringify(spatialFilters);
+    if (spatialDataType) queryParams.spatialDataType = spatialDataType;
+    if (spatialDataColumn) queryParams.spatialDataColumn = spatialDataColumn;


This only specifies which is the spatialDataColumn and shouldn't affect other Widgets, but it's used from the Backend perspective for determining which column they should "transform", for more info see: https://github.com/CartoDB/cloud-native/pull/17146

jmgaya · 2024-09-06T11:18:43Z

packages/react-widgets/src/widgets/TableWidget.js

@@ -121,6 +122,7 @@ function TableWidget({
            height={height}
            dense={dense}
            isLoading={isLoading}
+            onRowClick={onRowClick}


I've added support for the onRowClick event Handler to the TableWidget, because it needs support for this initiative 👍

ok but add a new comment in the parm section please

github-actions · 2024-09-06T11:39:51Z

Pull Request Test Coverage Report for Build 10770948415

Details

19 of 22 (86.36%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.007%) to 70.964%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/react-api/src/api/model.js	19	22	86.36%

Totals
Change from base Build 10722808249:	0.007%
Covered Lines:	2839
Relevant Lines:	3682

💛 - Coveralls

github-actions · 2024-09-06T11:40:40Z

Visit the preview URL for this PR (updated for commit 9d90a71):

https://cartodb-fb-storybook-react-dev--pr900-feature-438209-s-tkhjm3e2.web.app

_{(expires Mon, 16 Sep 2024 10:06:45 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: 517cc4d31d7e09cf277774e034094b67c301cd4c}

zbigg · 2024-09-06T12:46:43Z

packages/react-api/src/api/model.js

+    if (props.model === 'table') {
+      queryParams.params = JSON.stringify({
+        ...params,
+        column: [...params.column, spatialDataColumn]


Very minor.
I would prefer special parameter that either hides (hiddenColumns) some column or fetches extra column (invisiableColumns) than magic that just adds geo column to fetched set.

App will have power to use whatever "extra" column it needs - even if current use case is to add exactly same column as spatialDataColumn

I like it 🤔

The only "problem" I see is that we'll have to expose or move somewhere the extraction of the spatialDataColumn, because it's the one we need to pass down as "hiddenColumns` or whatever name we decide to use, WDYT?

I've tried to keep the number of changes as minimal as possible because of being Carto4React, but I'm happy to make more obvious the retrieval of such columns, rather than hiding magic in the "core utility"

Will share more details in Slack, but we are also working on adding the ability for the server to return a unique ID for all layers, by hashing the geometry. When a unique ID is available, perhaps fetching the entire geometry into the table widget is not necessary?

Agree with @donmccurdy but it's something that we need to check in the server-side click initiative. Also, I'm worried if PS uses this widget and suddenly they will start receiving geometries. @jmgaya It would be possible to request the geometries as an optional parameter of the table widget?

I think this if is going to be difficult to adapt when we integrate carto-api-client. The TableWidget has a columns parameter that you could use to add the spatialDataColumn and you would need an extra property to hide this column. I know that it requires more changes but I think that would be cleaner.

When a unique ID is available, perhaps fetching the entire geometry into the table widget is not necessary?

Yup, I think this makes total sense! However, it's not ready yet, but we should probably re-visit this once we merge this 👍

I think this if is going to be difficult to adapt when we integrate carto-api-client

Absolutely agree! I've tried to keep this as minimal as possible, but in terms of reusability, we can handle this in a different way, let me propose something else 👍

padawannn · 2024-09-09T07:58:39Z

packages/react-widgets/src/widgets/TableWidget.js

@@ -121,6 +122,7 @@ function TableWidget({
            height={height}
            dense={dense}
            isLoading={isLoading}
+            onRowClick={onRowClick}


ok but add a new comment in the parm section please

padawannn · 2024-09-09T08:23:41Z

packages/react-api/src/api/model.js

+    if (props.model === 'table') {
+      queryParams.params = JSON.stringify({
+        ...params,
+        column: [...params.column, spatialDataColumn]


Agree with @donmccurdy but it's something that we need to check in the server-side click initiative. Also, I'm worried if PS uses this widget and suddenly they will start receiving geometries. @jmgaya It would be possible to request the geometries as an optional parameter of the table widget?

padawannn · 2024-09-09T08:37:12Z

packages/react-api/src/api/model.js

+    if (props.model === 'table') {
+      queryParams.params = JSON.stringify({
+        ...params,
+        column: [...params.column, spatialDataColumn]


I think this if is going to be difficult to adapt when we integrate carto-api-client. The TableWidget has a columns parameter that you could use to add the spatialDataColumn and you would need an extra property to hide this column. I know that it requires more changes but I think that would be cleaner.

Retrieve spatialDataColumn in TableWidget for its future usage

e9b5419

jmgaya commented Sep 6, 2024

View reviewed changes

jmgaya requested review from zbigg, donmccurdy and padawannn September 6, 2024 11:18

Add tests for Table Widget and fix other ones

293165d

zbigg reviewed Sep 6, 2024

View reviewed changes

padawannn requested changes Sep 9, 2024

View reviewed changes

jmgaya added 2 commits September 9, 2024 11:55

PR Feedback

1b4d4fe

Remove unnecessary test

d464692

jmgaya requested review from zbigg and padawannn September 9, 2024 09:58

jmgaya marked this pull request as ready for review September 9, 2024 09:58

CHANGELOG.md

9d90a71

padawannn approved these changes Sep 10, 2024

View reviewed changes

jmgaya merged commit 04b9491 into master Sep 10, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Retrieve spatialDataColumn in TableWidget for its future usage #900

Retrieve spatialDataColumn in TableWidget for its future usage #900

jmgaya commented Sep 6, 2024

jmgaya Sep 6, 2024

jmgaya Sep 6, 2024

jmgaya Sep 6, 2024 •

edited

Loading

padawannn Sep 9, 2024

github-actions bot commented Sep 6, 2024 •

edited

Loading

github-actions bot commented Sep 6, 2024 •

edited

Loading

zbigg Sep 6, 2024

jmgaya Sep 6, 2024 •

edited

Loading

donmccurdy Sep 6, 2024

padawannn Sep 9, 2024

padawannn Sep 9, 2024

jmgaya Sep 9, 2024

jmgaya Sep 9, 2024

padawannn Sep 9, 2024

padawannn Sep 9, 2024

padawannn Sep 9, 2024

Retrieve spatialDataColumn in TableWidget for its future usage #900

Retrieve spatialDataColumn in TableWidget for its future usage #900

Conversation

jmgaya commented Sep 6, 2024

Description

Type of change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmgaya Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 6, 2024 • edited Loading

Pull Request Test Coverage Report for Build 10770948415

Details

💛 - Coveralls

github-actions bot commented Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

jmgaya Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmgaya Sep 6, 2024 •

edited

Loading

github-actions bot commented Sep 6, 2024 •

edited

Loading

github-actions bot commented Sep 6, 2024 •

edited

Loading

jmgaya Sep 6, 2024 •

edited

Loading