Fix #341: Added highlight function for Choropleth#490
Fix #341: Added highlight function for Choropleth#490ocefpaf merged 20 commits intopython-visualization:masterfrom
Conversation
…y for choropleth maps
…y for choropleth maps
| _default_js = [ | ||
| ('leaflet', | ||
| "https://cdnjs.cloudflare.com/ajax/libs/leaflet/0.7.3/leaflet.js"), | ||
| "https://npmcdn.com/leaflet@1.0.0-rc.3/dist/leaflet.js"), |
There was a problem hiding this comment.
We should update leaflet but a release candidate is not OK for a folium release!
There was a problem hiding this comment.
Totally, I can change back to the current stable leaflet build.
There was a problem hiding this comment.
Thanks! I will take a looking into testing the latest leaflet later. I will reserve this weekend for folium.
There was a problem hiding this comment.
I have changed back to stable build
There was a problem hiding this comment.
That is a release candidate, no? rc.3?
Still releases candidates are "stable enough." Let me just run the whole gallery with that version before merging this to be sure nothing is broken.
|
@joshuacano thanks for the PR! Can you please:
|
|
Absolutely @ocefpaf . For some reason I never got notified about these comments, would have done em sooner. I'll make those changes specified. |
|
Thanks. Things here are slow but (hopefully) steady. I am planning on a new release and this change will be an important addition. |
|
Sorry its taking me so long to address this, Is this gating your release??! I'll get to it ASAP this weekend. |
I would love to say so and find a scapegoat 😉 but no. I am overwhelmed with my day job and there are many other issues I need to address in So do not worry and take you time. |
|
Question. Is this going to always be on for choropleth? Or should we have a switch in the method that specifies if this feature is on? (I.e. highlight=False in choropleth method signature) I'm gonna go with the assumption that it's always on. (Will just require a minor tweak to an existing test) |
|
Two more questions @ocefpaf,
|
Let's keep it turned off to preserve the current behavior. Specially b/c this has the potential to be expanded later on for other "functions" beyond the highlight. (Still thinking about this but I haven't got the time to look into it.)
That is up to you. (Maybe a new PR might be better to keep things simple.) ? 2. Are there any selenium type tests in the test suite? I was trying to figure out how to test that when the user mouses over a region that the highlight function is called. But without any selenium type tests I'm not sure how to do so. Perhaps you have some guidance on how I can approach that? Nope. Maybe we do not need to be that complex 😄 |
|
Got IT! I'll add a switch (as I should have seen in your earlier comment) and leave the default as off and add a test as well as an extra example to the notebook. Will push this shortly. Furthermore, good point on the new PR, I'll do that for a new legend once I get these other items taken care of. Thanks a bunch @ocefpaf! |
|
I have added a test for highlight and added a highlight switch to the choropleth method. I also added some cells to the existing notebooks in the example directory. Is this along the lines of what you were looking for? Let me know I'll be glad to change. |
|
@joshuacano looks good to me. There are conflicts here. Can you rebase and fix them? If GitHub rebasing is not your forte let me know and I will send a new PR picking your commits. |
|
I'll give the rebasing a shot. |
|
@ocefpaf Alright, I think I have fixed the conflicts, do you mind reviewing for me? |
| "metadata": { | ||
| "collapsed": false | ||
| }, | ||
| <<<<<<< HEAD |
There was a problem hiding this comment.
It seems we have a conflict remaining here. Don't worry I will fix this in another PR.
| "metadata": { | ||
| "collapsed": true | ||
| }, | ||
| "outputs": [], |
There was a problem hiding this comment.
It seems that this cell was not executed.
|
@joshuacano let's keep this in and I will fix the remaining issues. Thanks for the PR! Hope to see more 😜 |
Fix python-visualization#341: Added highlight function for Choropleth
3 participants
This is how I was thinking of implementing this, just a minor tweak on your solution @BibMartin. I currently have hardcoded 2 and .2 for weight and fillOpacity for each shape that is highlighted. Let me know how this looks? Also I was hoping to had a legend functionality to add the custom info control functionality if desired by the user as well. (as described in the custom info section towards the bottom of this page http://leafletjs.com/examples/choropleth.html) Is this something that is wanted also? Should we include this. Thanks for reviewing this in advance.