Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: Map visualization (using Leaflet) #466

Merged
merged 6 commits into from
Jul 1, 2015

Conversation

BrunoSalerno
Copy link
Contributor

@arikfr I open the PR.
This is a map visualization that uses leaflet. Right now only supports point features. You can select the coordinates columns, and draw the points using markers or colored circles.
Each feature has a popup that shows all the 'columns' of the row.

btw: I didn't know what to do with the size of the main div inside the template map.html

(Closes #39)

@landscape-bot
Copy link

Code Health
Repository health increased by 0.15% when pulling b913ce6 on BrunoSalerno:leaflet-visualization into 9cdc2cb on EverythingMe:master.

@@ -0,0 +1 @@
<div style='margin:20px;height:500px;width:500px;'></div>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can it inherit width from the containing element? So the user can control the width like he can for the other widgets.

As for height, maybe change this to be a visualization property (with some default value)?

@arikfr
Copy link
Member

arikfr commented Jul 1, 2015

This is really great. Let's get the width/height sorted, and merge :)

@arikfr arikfr changed the title Map visualization (using leaflet) Feature: Map visualization (using Leaflet) Jul 1, 2015
@BrunoSalerno
Copy link
Contributor Author

@arikfr great! I'm working on it

@BrunoSalerno
Copy link
Contributor Author

@arikfr done. I couldn't use VisualizationProvider.defaultOptions. Is it alright this way or there is a specific way to use defaultOptions?

@landscape-bot
Copy link

Code Health
Repository health increased by 0.15% when pulling d7fb2d7 on BrunoSalerno:leaflet-visualization into 9cdc2cb on EverythingMe:master.

@arikfr
Copy link
Member

arikfr commented Jul 1, 2015

@BrunoSalerno what do you mean you couldn't use them?

});

if (!$scope.map) {
$(elm[0].children[0]).replaceWith($("<div style='margin:1%;height:"+$scope.visualization.options.height+"px;width:95%;'></div>"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need jQuery here, you can just change the template to:

<div style='margin:1%;width:95%;height:{{visualization.options.height}}px;'></div>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've played around with your branch a little, and noticed that when height is being changed this way (dynamically), it's better to have the map in an inner div. So the template becomes:

<div style='margin:1%;width:95%;height:{{visualization.options.height}}px;'>
    <div style="width:100%; height:100%;"></div>
</div>

And line 138 changes to: $scope.map = L.map(elm[0].children[0].children[0]);.

And you need to add watch for height:

              $scope.$watch('visualization.options.height', function() {

                  $scope.map.invalidateSize(false);
                  setBounds();

              });

@BrunoSalerno
Copy link
Contributor Author

@arikfr I set defaultOptions in VisualizationProvider, so I expect them to be set in $scope.visualization.options, but they are not. Am I missing something?

<div class="form-group">
<label class="col-lg-2">Map height (px)</label>
<div class="col-sm-4">
<input class="form-control" type="text" ng-model = "visualization.options.height" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to have here type="number" (then it's being saved as number, and you can control the value with arrows).

@arikfr
Copy link
Member

arikfr commented Jul 1, 2015

@BrunoSalerno re. defaultOptions, looks like it works only for the default visualization type. I'll fix it an update your code to use it.

@BrunoSalerno
Copy link
Contributor Author

@arikfr nice. I'm pushing the changes and adding a FIXME in the editor directive (in the height part)

@arikfr
Copy link
Member

arikfr commented Jul 1, 2015

👍

@landscape-bot
Copy link

Code Health
Repository health increased by 0.15% when pulling b743cce on BrunoSalerno:leaflet-visualization into 9cdc2cb on EverythingMe:master.

arikfr added a commit that referenced this pull request Jul 1, 2015
Feature: Map visualization (using Leaflet)
@arikfr arikfr merged commit e04833c into getredash:master Jul 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants