[deck polygon] add support for geohash #5712

mistercrunch · 2018-08-23T16:36:00Z

improvements:

added autozoom support
support for metric & aggregations (only aggregates if metric is picked)
fixed stroke
fixed opacity
introduced a SliderControl

+ improvements: * added autozoom support * support for metric & aggregations (only aggregates if metric is picked) * fixed stroke * fixed opacity * introduced a SliderControl

kristw · 2018-08-23T16:50:12Z

superset/assets/src/modules/colors.js

-  if (!hex) {
-    return [0, 0, 0, alpha];
+  const scaler = d3.scale.linear().domain(points).range(colors).clamp(true);
+  if (outpoutRGBA) {


kristw · 2018-08-23T16:50:43Z

superset/assets/src/modules/colors.js

@@ -525,6 +525,16 @@ export const spectrums = {
  ],
 };

+export function hexToRGB(hex, alpha = 255) {


@hughhhh already imports d3-colors . Perhaps can use that?

Oh ere I only move the code from lower in the file since my linter was complaining about "referenced before defined". Though I don't want to sign up for it in the scope of this, PR, I totally agree that we should leverage d3-color as much as possible.

Got it. I am fine with leaving it for later refactor outside of this PR.

Yes actually I'm realizing that there's a need here for replacing whites with transparency for maps. That never was an issue with white backgrounds, but these maps would look better with transparency in place of whites. This calls for more of a refactor of these color gradients. We're also thinking about improving the control itself, allowing for user choosing colors and bounds, and maybe something about quantiles/ranking.

codecov-io · 2018-08-27T03:43:54Z

Codecov Report

Merging #5712 into master will decrease coverage by <.01%.
The diff coverage is 54.63%.

@@            Coverage Diff             @@
##           master    #5712      +/-   ##
==========================================
- Coverage   63.38%   63.37%   -0.01%     
==========================================
  Files         361      364       +3     
  Lines       23000    23038      +38     
  Branches     2559     2564       +5     
==========================================
+ Hits        14579    14601      +22     
- Misses       8406     8422      +16     
  Partials       15       15

Impacted Files	Coverage Δ
superset/assets/src/explore/visTypes.jsx	`57.14% <ø> (ø)`	⬆️
superset/assets/src/visualizations/word_cloud.js	`0% <ø> (ø)`	⬆️
superset/assets/src/explore/controls.jsx	`46.26% <ø> (ø)`	⬆️
superset/assets/src/visualizations/PlaySlider.jsx	`0% <0%> (ø)`	⬆️
...ssets/src/visualizations/deckgl/layers/polygon.jsx	`0% <0%> (ø)`	⬆️
.../assets/src/visualizations/deckgl/layers/common.js	`0% <0%> (ø)`	⬆️
...sets/src/visualizations/deckgl/DeckGLContainer.jsx	`0% <0%> (ø)`	⬆️
superset/assets/src/chart/Chart.jsx	`57.69% <100%> (ø)`	⬆️
...et/assets/src/explore/components/controls/index.js	`100% <100%> (ø)`	⬆️
superset/views/core.py	`73.93% <100%> (ø)`	⬆️
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f83ea23...3e8d857. Read the comment docs.

betodealmeida

LGTM, just a few comments.

betodealmeida · 2018-08-28T01:50:38Z

superset/assets/src/explore/controls.jsx

+    min: 0,
+    max: 100,
+    renderTrigger: true,
+    description: t('Opacity, expects values between 1 and 100'),


Nit: "...between 0 and 100"

betodealmeida · 2018-08-28T01:54:27Z

superset/assets/src/visualizations/PlaySlider.jsx

@@ -4,9 +4,7 @@ import { Row, Col } from 'react-bootstrap';

 import Mousetrap from 'mousetrap';

-import 'bootstrap-slider/dist/css/bootstrap-slider.min.css';
-import ReactBootstrapSlider from 'react-bootstrap-slider';
-import './PlaySlider.css';


This import should still be here, since it has more styles than were moved to BootrapSliderWrapper.

Good catch!

betodealmeida · 2018-08-28T01:54:57Z

superset/assets/src/visualizations/deckgl/DeckGLContainer.jsx

@@ -7,14 +7,15 @@ import 'mapbox-gl/dist/mapbox-gl.css';
 const propTypes = {
  viewport: PropTypes.object.isRequired,
  layers: PropTypes.array.isRequired,
-  setControlValue: PropTypes.func.isRequired,
+  setControlValue: PropTypes.func,


Thanks, this was annoying.

betodealmeida · 2018-08-28T01:55:57Z

superset/assets/src/visualizations/deckgl/layers/common.js

  if (fd.js_tooltip) {
-    const jsTooltip = sandboxedEval(fd.js_tooltip);
+    tooltipContentGenerator = o => dompurify.sanitize(sandboxedEval(fd.js_tooltip)(o));


tooltipContentGenerator = dompurify.sanitize(sandboxedEval(fd.js_tooltip));

I think this is right as is. We need to sanitize the output of sandboxedEval(fd.js_tooltip)(o). The line is heavy though. I can make it 2 lines if you think there's too much going on.

Oh, you're right. My bad.

* [deck polygon] add support for geohash + improvements: * added autozoom support * support for metric & aggregations (only aggregates if metric is picked) * fixed stroke * fixed opacity * introduced a SliderControl * addressing comments, fixing build * Addressing comments (cherry picked from commit 60ecd72)

* [deck polygon] add support for geohash + improvements: * added autozoom support * support for metric & aggregations (only aggregates if metric is picked) * fixed stroke * fixed opacity * introduced a SliderControl * addressing comments, fixing build * Addressing comments

* [deck polygon] add support for geohash + improvements: * added autozoom support * support for metric & aggregations (only aggregates if metric is picked) * fixed stroke * fixed opacity * introduced a SliderControl * addressing comments, fixing build * Addressing comments (cherry picked from commit 60ecd72)

* [deck polygon] add support for geohash + improvements: * added autozoom support * support for metric & aggregations (only aggregates if metric is picked) * fixed stroke * fixed opacity * introduced a SliderControl * addressing comments, fixing build * Addressing comments

[deck polygon] add support for geohash

63d25dd

+ improvements: * added autozoom support * support for metric & aggregations (only aggregates if metric is picked) * fixed stroke * fixed opacity * introduced a SliderControl

kristw reviewed Aug 23, 2018

View reviewed changes

mistercrunch force-pushed the polygon_geohash branch from 48a8fb0 to 8a011d6 Compare August 24, 2018 18:27

addressing comments, fixing build

1482e33

mistercrunch force-pushed the polygon_geohash branch from 8a011d6 to 1482e33 Compare August 27, 2018 00:20

betodealmeida requested changes Aug 28, 2018

View reviewed changes

Addressing comments

3e8d857

betodealmeida approved these changes Aug 28, 2018

View reviewed changes

betodealmeida merged commit 60ecd72 into apache:master Aug 28, 2018

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[deck polygon] add support for geohash #5712

[deck polygon] add support for geohash #5712

mistercrunch commented Aug 23, 2018

kristw Aug 23, 2018

mistercrunch Aug 23, 2018

kristw Aug 23, 2018

mistercrunch Aug 23, 2018

kristw Aug 27, 2018

mistercrunch Aug 27, 2018

codecov-io commented Aug 27, 2018 •

edited

Loading

betodealmeida left a comment

betodealmeida Aug 28, 2018

betodealmeida Aug 28, 2018

mistercrunch Aug 28, 2018

betodealmeida Aug 28, 2018

betodealmeida Aug 28, 2018

mistercrunch Aug 28, 2018

betodealmeida Aug 28, 2018

[deck polygon] add support for geohash #5712

[deck polygon] add support for geohash #5712

Conversation

mistercrunch commented Aug 23, 2018

improvements:

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-io commented Aug 27, 2018 • edited Loading

Codecov Report

betodealmeida 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Aug 27, 2018 •

edited

Loading