Add: map marker cluster visualization #1203

WesleyBatista · 2016-07-22T18:35:18Z

This pull request adds Leaflet.markercluster visualization, using ui-leaflet directive from angular-ui.

Known issues:

We have a limit of 10000 rows. To prevent the web browser to freeze building the points we don't let the script runs if the query results has more than 10k lines. Keep in mind that even with this limit your browser or the final user browser can freeze because it depends on the RAM memory available on the device used. A reasonable number on my tests was 5~6K :/
The screen to edit the visualization has UI/UX problems. Trying to provide a fully configurable visualization I exposed all the settings available. This led the UI to have options that doesn't make sense. Also, changing the values do not make them appear on the pre-visualization aside.

Screenshots

Mobile

arikfr · 2016-07-24T07:59:57Z

rd_ui/app/vendor_scripts.html

@@ -39,6 +39,9 @@
 <script src="/bower_components/canvg/StackBlur.js"></script>
 <script src="/bower_components/canvg/canvg.js"></script>
 <script src="/bower_components/leaflet/dist/leaflet.js"></script>
+<script src="/bower_components/angular-simple-logger/dist/angular-simple-logger.js"></script>


I think you forgot to remove this.

This is a dependency from angular-ui/ui-leaflet :/

arikfr · 2016-07-24T08:11:17Z

Thanks! Looking good and definitely will be useful.

The points limit is reasonable and it's good you put a "safe guard" in place.

The screen to edit the visualization has UI/UX problems. Trying to provide a fully configurable visualization I exposed all the settings available. This led the UI to have options that doesn't make sense.

I suggest to review the options and remove the ones that are less likely to be used. Also, let's split it into several tabs, like the charts visualization has. So the less common options won't confuse the "regular" user and be hidden in their own tab.

Also, changing the values do not make them appear on the pre-visualization aside.

That should be easy to fix, let's revisit it after implementing the tabs?

Btw, do you think it will be possible to merge this with the other map visualization?

WesleyBatista · 2016-07-25T17:17:09Z

I will try to implement the tabs and remove some options until tomorrow and send the changes.

WesleyBatista · 2016-07-25T17:17:54Z

Regarding merging with the nowadays solution I think that is a good idea and it was a thing that I was intended to do on the conception.
But I found a little bit tricky since the map visualization is currently using the Leaflet library directly on the visualization code to implement it, and my solution takes the angular-ui's directive.

WesleyBatista · 2016-07-25T17:23:58Z

I think it is possible to merge (and for sure I do like to help on this), but for now maybe it would be better to do this later in another PR.
Do you agree?

WesleyBatista · 2016-07-25T23:59:30Z

@arikfr, can you help me with the options to be updated on the pre-visualization?
It is something with the $scope.$watch. I am getting an infinite loop :/

arikfr · 2016-07-27T18:42:04Z

can you help me with the options to be updated on the pre-visualization? It is something with the $scope.$watch. I am getting an infinite loop :/

Sure I'll give it a look.

Currently limiting the number of points to 6K. This is to do not freeze the machine due to a kind of memory leak. Take a look at: angular-ui/ui-leaflet#19

- change the 'renderTemplate'; - add more options to 'defaultOptions'; - add a set of baselayers options found on [here](https://leaflet-extras.github.io/leaflet-providers/preview/); - ability to set the marker text using a column 'msg' on query; - remove the comments from 'defaultOptions' - log a message if the points exceeds 10k

- remove the sample data and get the configs from visualization obj - breakdown the tag for readability

- get the lat/lon columns from the visualization configs - add 'descColName' option to visualization configs

- Organize options in tabs - Remove the width option

WesleyBatista · 2016-08-15T21:26:29Z

@arikfr, I was thinking about do not call the type of this visualization 'MAP_MARKERCLUSTER'.
What do you think about something more general like 'MAP_PLOT', 'MAP_VIZ'... I don't know. But call it something more general and start a smoothly migration from the old to this new one.

Prefer HTTPS instead of SSH protocol

zlu · 2016-08-16T18:16:12Z

+1

Update markercluster.js - Add default values for 'chunked' options - Fix the https issue requesting the tiles Update markercluster_editor.html - Add tooltip helpers - Remove link to github source projects READMEs

arikfr · 2016-09-05T07:46:39Z

I finally had the time to look into it:

Also, changing the values do not make them appear on the pre-visualization aside.

There are two issues here:

You are changing the Scope's visualization object to be the visualization.options object, hence breaking the reference in the height setting.
It seems that this directive doesn't support changing options after rendering: it applies the defaults options at the link stage of the controller, which is being run only once.

While reviewing their code, it looks like 5K LOC that have built in support for many Leaflet's plugins. Including duplicate plugins: like support for 5 different icons plugins, that do pretty much the same thing.

The lack of support for "live preview", redundant code and other factors lead me to think of dropping angular-ui-leaflet and keep using Leaflet directly. If you don't mind, I would like to take a stab at implementing the Marker Cluster functionality directly on Leaflet (along with refactoring the current map visualization). OK?

arikfr · 2016-09-22T20:15:50Z

As discussed with @WesleyBatista, I'm closing this in favor of #1292, where I implement clustering using Leaflet.markersCluster directly instead of this Angular wrapper. The main reason being that the wrapper doesn't support some features we need (like live preview).

arikfr reviewed Jul 24, 2016
View reviewed changes

arikfr changed the title ~~Marker Cluster visualization~~ Add: map marker cluster visualization Jul 24, 2016

Wesley Batista and others added 21 commits August 15, 2016 15:21

Add Marker Cluster visualization dependencies to bower.json

3c207a9

Import stylesheets required by leaflet.markercluster lib

bbfacfd

Import vendor scripts required by leaflet directive

dc08f01

Add 'ui-leaflet' as a module required by the main application module

6677743

Add Marker Cluster visualization files

6d481d7

Currently limiting the number of points to 6K. This is to do not freeze the machine due to a kind of memory leak. Take a look at: angular-ui/ui-leaflet#19

Import markercluster.js

184cf62

Add markercluster defaultOptions to visualization script

0034051

Update the markerclusters template html file

7886651

- remove the sample data and get the configs from visualization obj - breakdown the tag for readability

Add the markerclusters visualization editor template

73a9755

Update markercluster.js

f44b6b0

- get the lat/lon columns from the visualization configs - add 'descColName' option to visualization configs

Change the markercluster visualization name

5c01b05

Add center options to markercluster visualization

fdaf9b9

Change markercluster visualization 'scrollWheelZoom' default value

806df38

Fix: min/max zoom markercluster configs set to right place

357c02a

Fix: remove console.log

1dc976d

Add growl to markercluster visualization and update message

2bd5cdf

Set markercluster data only when the QueryResult promise ended

3859842

Fix typo on the markercluster message

f4a743a

Fix markercluster width to 100%

a170e1f

Update markercluster visualization editor

9251b6c

- Organize options in tabs - Remove the width option

Change the leaflet.markercluster URL on bower.json

dd0669c

Prefer HTTPS instead of SSH protocol

Wesley Batista added 5 commits August 16, 2016 18:08

Add ability to set the default baselayer

c70d5bf

Add baselayers

b2e5ca9

Add images required by leaflet

619ecbb

Update MarkerCluster visualization

d5286d5

Update markercluster.js - Add default values for 'chunked' options - Fix the https issue requesting the tiles Update markercluster_editor.html - Add tooltip helpers - Remove link to github source projects READMEs

Resolve merge conflicts with getredash/master

2371394

arikfr mentioned this pull request Sep 22, 2016

Change: improvements to map visualization #1292

Merged

arikfr closed this Sep 22, 2016

WesleyBatista deleted the marker-cluster-visualization branch December 6, 2016 12:26

WesleyBatista restored the marker-cluster-visualization branch December 6, 2016 12:26

MaxMood96 mentioned this pull request Oct 2, 2023

[Snyk] Security upgrade css-loader from 0.28.11 to 5.0.0 MaxMood96/redash#116

Open

Ariffirdausazman mentioned this pull request Dec 22, 2023

[Snyk] Fix for 46 vulnerabilities Reflektion/redash#4

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add: map marker cluster visualization #1203

Add: map marker cluster visualization #1203

WesleyBatista commented Jul 22, 2016

arikfr Jul 24, 2016

WesleyBatista Jul 25, 2016

arikfr commented Jul 24, 2016

WesleyBatista commented Jul 25, 2016

WesleyBatista commented Jul 25, 2016

WesleyBatista commented Jul 25, 2016

WesleyBatista commented Jul 25, 2016

arikfr commented Jul 27, 2016

WesleyBatista commented Aug 15, 2016

zlu commented Aug 16, 2016

arikfr commented Sep 5, 2016

arikfr commented Sep 22, 2016

Add: map marker cluster visualization #1203

Add: map marker cluster visualization #1203

Conversation

WesleyBatista commented Jul 22, 2016

Known issues:

Screenshots

Mobile

arikfr Jul 24, 2016

Choose a reason for hiding this comment

WesleyBatista Jul 25, 2016

Choose a reason for hiding this comment

arikfr commented Jul 24, 2016

WesleyBatista commented Jul 25, 2016

WesleyBatista commented Jul 25, 2016

WesleyBatista commented Jul 25, 2016

WesleyBatista commented Jul 25, 2016

arikfr commented Jul 27, 2016

WesleyBatista commented Aug 15, 2016

zlu commented Aug 16, 2016

arikfr commented Sep 5, 2016

arikfr commented Sep 22, 2016