feat: Add line width unit control in deckgl Polygon and Path #24755

kgabryje · 2023-07-20T13:26:02Z

SUMMARY

Currently the default unit of line width in deckgl Path and Polygon charts is meter. That leads to some confusion, especially in large scale maps, because users might think that the line is not drawn at all - while in reality it's simply too thin to be visible. This PR addresses that problem by changing the default unit from meters to pixels.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2023-07-20.at.15.23.21.mov

TESTING INSTRUCTIONS

Create a deckgl Path chart
Verify that the default line width unit is pixels and default line width is 1 and that drawn line is visible
Verify a deckgl Polygons chart
Check "Stroked" and uncheck "Extruded"
Verify that line between polygons is visible
Verify that existing charts still use meters unit

ADDITIONAL INFORMATION

Has associated issue:
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
  Current: 0.12 s
  10+: 0.12 s
  100+: 0.21 s
  1000+: 0.13 s
Introduces new feature or API
Removes existing feature or API

codecov · 2023-07-20T16:48:03Z

Codecov Report

Merging #24755 (99e2ae1) into master (b8a3eef) will increase coverage by 1.74%.
The diff coverage is n/a.

❗ Current head 99e2ae1 differs from pull request most recent head f517ac6. Consider uploading reports for the commit f517ac6 to get more accurate results

@@            Coverage Diff             @@
##           master   #24755      +/-   ##
==========================================
+ Coverage   67.17%   68.92%   +1.74%     
==========================================
  Files        1902     1902              
  Lines       73939    73939              
  Branches     8176     8176              
==========================================
+ Hits        49672    50965    +1293     
+ Misses      22154    20861    -1293     
  Partials     2113     2113

Flag	Coverage Δ
hive	`54.14% <ø> (ø)`
javascript	`55.76% <ø> (ø)`
mysql	`79.19% <ø> (ø)`
postgres	`79.28% <ø> (-0.02%)`	⬇️
presto	`54.03% <ø> (?)`
python	`83.30% <ø> (+3.65%)`	⬆️
sqlite	`?`
unit	`54.96% <ø> (?)`

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

Impacted Files	Coverage Δ
...et-chart-deckgl/src/layers/Geojson/controlPanel.ts	`50.00% <ø> (ø)`
...egacy-preset-chart-deckgl/src/layers/Path/Path.jsx	`0.00% <ø> (ø)`
...reset-chart-deckgl/src/layers/Path/controlPanel.ts	`50.00% <ø> (ø)`
...preset-chart-deckgl/src/layers/Polygon/Polygon.jsx	`0.00% <ø> (ø)`
...et-chart-deckgl/src/layers/Polygon/controlPanel.ts	`33.33% <ø> (ø)`
...reset-chart-deckgl/src/utilities/Shared_DeckGL.jsx	`86.48% <ø> (ø)`
superset/examples/deck.py	`0.00% <ø> (ø)`

... and 103 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

john-bodley · 2023-07-21T18:08:00Z

superset/migrations/versions/2023-07-19_17-54_ee179a490af9_deckgl_path_width_units.py

+            Slice.viz_type == "deck_polygon",
+        )
+    ):
+        params = json.loads(slc.params)


@kgabryje would you mind adding a try/except around the json.loads(...) block as there could be instances where the JSON payload is corrupted. See #24737 for details.

john-bodley · 2023-07-21T18:08:22Z

superset/migrations/versions/2023-07-19_17-54_ee179a490af9_deckgl_path_width_units.py

+        if not params.get("line_width_unit"):
+            params["line_width_unit"] = "meters"
+        slc.params = json.dumps(params)
+        session.merge(slc)


There's no need to do a session.merge(...) here. Typically merge is used when you may have more than one in-memory objects which map to the same database record with some key.

Thanks for explanation!

john-bodley · 2023-07-21T18:10:34Z

superset/migrations/versions/2023-07-19_17-54_ee179a490af9_deckgl_path_width_units.py

+        params = json.loads(slc.params)
+        if not params.get("line_width_unit"):
+            params["line_width_unit"] = "meters"
+        slc.params = json.dumps(params)


Also per #24737 it's probably best to only update slc.params if the parameters were updated. There's likely only a handful of charts which need upgrading and thus there's no need to update them all—which adds unnecessary effort on the database.

I assume that this will impact nearly all charts that are found by the filter clause. But I agree, adding the indentation here is more prudent and will avoid unnecessary db work.

Yes that's exactly the case, but you're right - for the sake of code quality I added the indent

villebro

One remark to complement John's review comments.

villebro · 2023-07-24T07:35:53Z

superset/examples/deck.py

@@ -387,6 +387,7 @@ def load_deck_dash() -> None:  # pylint: disable=too-many-statements
        "stroked": False,
        "extruded": True,
        "multiplier": 0.1,
+        "line_width": 10,


Shouldn't we also add "line_width_unit": "meters" here, as the default unit is now being changed?

Great catch!

villebro · 2023-07-24T07:40:24Z

superset/migrations/versions/2023-07-19_17-54_ee179a490af9_deckgl_path_width_units.py

+        params = json.loads(slc.params)
+        if not params.get("line_width_unit"):
+            params["line_width_unit"] = "meters"
+        slc.params = json.dumps(params)


I assume that this will impact nearly all charts that are found by the filter clause. But I agree, adding the indentation here is more prudent and will avoid unnecessary db work.

villebro · 2023-07-25T15:14:03Z

superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Polygon/controlPanel.ts

-        [extruded, multiplier],
-        [lineWidth, null],
+        [extruded],
+        [multiplier],
+        [lineWidth],


Finally these are split onto their own rows 👏

I'm going to do the same in the rest of deckgl plugins in upcoming PRs, no more controls squished in 1 row 😄

…24755)

kgabryje requested review from yousoph and villebro July 20, 2023 13:26

kgabryje requested a review from a team as a code owner July 20, 2023 13:26

pull-request-size bot added the size/L label Jul 20, 2023

kgabryje force-pushed the feat/deckgl-path-width-units branch from 00ea75d to a2bb699 Compare July 20, 2023 16:20

john-bodley reviewed Jul 21, 2023

View reviewed changes

villebro reviewed Jul 24, 2023

View reviewed changes

kgabryje added 6 commits July 24, 2023 15:13

Add line width unit control to Path

9e2011d

Add line width unit to Polygons

9ea7588

Update down revision id

c6c6a17

Optimise migration

6ce55e3

Add line width unit to example chart

f4ed738

Update down revision id

f517ac6

kgabryje force-pushed the feat/deckgl-path-width-units branch from 2c59e03 to f517ac6 Compare July 24, 2023 13:15

villebro approved these changes Jul 27, 2023

View reviewed changes

kgabryje merged commit d26ea98 into apache:master Jul 27, 2023

yousoph mentioned this pull request Aug 2, 2023

Update Path_68.yaml default line width unit preset-io/public-examples#19

Merged

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.1.0 labels Mar 8, 2024

vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 2024

feat: Add line width unit control in deckgl Polygon and Path (apache#…

97dff3f

…24755)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add line width unit control in deckgl Polygon and Path #24755

feat: Add line width unit control in deckgl Polygon and Path #24755

kgabryje commented Jul 20, 2023

codecov bot commented Jul 20, 2023 •

edited

Loading

john-bodley Jul 21, 2023

john-bodley Jul 21, 2023

kgabryje Jul 24, 2023

john-bodley Jul 21, 2023

villebro Jul 24, 2023

kgabryje Jul 24, 2023

villebro left a comment

villebro Jul 24, 2023

kgabryje Jul 24, 2023

villebro Jul 24, 2023

villebro Jul 25, 2023

kgabryje Jul 27, 2023

feat: Add line width unit control in deckgl Polygon and Path #24755

feat: Add line width unit control in deckgl Polygon and Path #24755

Conversation

kgabryje commented Jul 20, 2023

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Jul 20, 2023 • edited Loading

Codecov Report

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

villebro left a 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

codecov bot commented Jul 20, 2023 •

edited

Loading