Add 'BeautifyMarker' marker type to the project. Rework from Pull ID: #776 #819

JeremyBYU · 2018-02-06T03:16:04Z

Hi,
I'm pretty new to doing a pull request so I hope I'm doing this right. This is basically the same pull request #776 from @arthuralvim, however I tried to finish all the requests/comments you gave before merging. I needed this work for my own research so I thought I would give it a go.

Comments/Issues from #776 hopefully resolved:

Remove BeautifyMarker class from folium/map.py to its own plugin
Dynamically load the CSS and JS
Add Tests (first time writing tests!)
Add an example of the BeautifyMarker plugin in examples\Plugins.ipynb
Started working on CHANGES.txt. Once a pull id is assigned and any fixes you need are done, I will update.

Most of this work is exactly from @arthuralvim, so the credit should mostly go to him. I wasn't sure how to pull from a 'pull request' and then resubmit under the same pull ID. So I have just forked master today and updated these changes. Let me know if there is any other work you need.

Thanks,

Jeremy

Conengmo · 2018-04-14T12:14:22Z

Hi @JeremyBYU,

I reviewed your PR, it's a good addition. Great that you included tests and some examples. I have some suggestions, let me know what you think. I hope you're still willing to work on this!

Could you remove the .gitignore file from the branch? There currently is no .gitignore file in folium, if we add one it should be done separately.
If you update from master the Travis tests should succeed.
Currently part of the code is shared with Marker which would be good to avoid. Since this addition is only about icons, I think it would be best to model this class after the Icon class. That would be most similar to what users are currently used to:

BeautifyIcon(some_options).add_to(marker_object)
Marker(icon=BeautifyIcon(some_options))

I would suggest to parse the options similarly as in TileLayer. Now the options are in two places: they are stored in self (e.g. self.icon = icon) and then in the template they are added to an options variable. Directly adding them to self.options in the __init__ reduces lines. Furthermore the logic (if-statements) in the template are harder to read than if they were in the __init__. But I know this isn't done in folium consistently, and maybe there are also arguments against this, so I understand if you keep it like it is.
When moving the options to self.options, the has_number and is_alpha_numeric_icon can maybe be removed, since they have only one line and are only used once.
Is _get_self_bounds() used anywhere?

JeremyBYU · 2018-04-19T19:28:08Z

I'll take a look at it later next week. I think the requests are all doable.

stickler-ci · 2018-04-25T01:39:02Z

tests/plugins/test_beautify_icon.py

+    bm1 = folium.Marker(location=[46, -122],
+                        popup='Portland, OR',
+                        icon=ic1
+                       ).add_to(m)


E124 closing bracket does not match visual indentation

I can fix that. However I had no idea that I pushed my changes to the python-visualization/folium repository. Are any changes that I make on my fork automatically pushed to yours (folium)? Or did I accidentally somehow do a push to folium? I'm guessing the later, but not sure.

Delete gitignore and BeautifulMarker.py

JeremyBYU · 2018-04-25T02:09:50Z

Okay @Conengmo, I think this covered what you wanted. The only problem is that a bunch of commits occurred, and I'm not sure how it happened! I think I just realized that any push that I make to my fork will automatically sync up here (specifically because I'm in a pull request)! I had no idea!

Let me know if there's anything else I need to change.

Conengmo

Thanks for your work @JeremyBYU, it looks great! Code, tests, example in notebook, it's all good. I have some very small comments on the docstring format. Could you take a look at those? Should be easy to fix. After that we're good to go!

Conengmo · 2018-04-25T06:28:17Z

folium/plugins/beautify_icon.py

+class BeautifyIcon(MacroElement):
+    """
+    Create a BeautifyIcon that can be added to a Marker
+    Parameters


There should be a blank line before each heading. So before Parameters, Returns and Examples.

Conengmo · 2018-04-25T06:29:44Z

folium/plugins/beautify_icon.py

+    Create a BeautifyIcon that can be added to a Marker
+    Parameters
+    ----------
+    icon: string


If you want to make this perfect, you could add the default values to each item. Like in the normal Icon class.

Conengmo · 2018-04-25T06:31:37Z

folium/plugins/beautify_icon.py

+        the number of the icon.
+    Returns
+    -------
+    Icon names and HTML in obj.template_vars


This class doesn't return anything, so the Returns part should be removed. (Technically it returns None but for a class docstring this is not documented.)

Conengmo · 2018-04-25T06:33:18Z

folium/plugins/beautify_icon.py

+                                    inner_icon_style='font-size:12px;padding-top:-5px;')
+    >>> Marker(location=[45.5, -122.3], popup=folium.Popup('Portland, OR'), icon=number_icon)
+    >>> BeautifyIcon(icon='arrow-down', icon_shape='marker').add_to(marker)
+    """


Lastly, can you add a link to the plugin website here? It's this one right?
https://github.com/marslan390/BeautifyMarker

Conengmo · 2018-04-25T06:34:12Z

CHANGES.txt

@@ -10,6 +10,7 @@
 - Added `tooltip` support to all vector layers (ocefpaf #722)
 - Added `TimeSliderChoropleth` plugin (halfdanrump #736)
 - Added `show` parameter to choose which overlays to show on opening (conengmo #772)
+- Added BeautifyIcon Plugin (arthuralvim and jeremybyu #776)


The number of this PR is #819, could you add it?

JeremyBYU · 2018-04-25T14:24:42Z

Awesome thanks!

ocefpaf · 2018-04-25T14:29:45Z

Thanks @JeremyBYU for the PR and @Conengmo for the review! The BeautifyMarkeris an awesome addition to folium!

Created plugin, added test, added notebook example

e2b339b

Conengmo mentioned this pull request Apr 21, 2018

Numbers in map marker in Folium #734

Closed

Conengmo added the PR: changes needed label Apr 24, 2018

JeremyBYU added 2 commits April 24, 2018 15:41

Merge remote-tracking branch 'upstream/master'

0ad84ce

Changed BeautifyMarker to BeautifyIcon

df24d77

stickler-ci reviewed Apr 25, 2018

View reviewed changes

Frank and others added 4 commits April 24, 2018 21:44

max_native_zoom missing in test

2b9c403

Changed BeautifyMarker to BeautifyIcon

867d911

Delete gitignore and BeautifulMarker.py

Merge branch 'master' of https://github.com/JeremyBYU/folium

d4913c2

Fixed indentation issue

cf14c18

Conengmo reviewed Apr 25, 2018

View reviewed changes

Formatted docstrings, added link to plugin website.

9e9b296

Conengmo merged commit 065f6f3 into python-visualization:master Apr 25, 2018

Conengmo mentioned this pull request Apr 25, 2018

Add 'BeautifyMarker' marker type to the project. #776

Closed

Conengmo removed the PR: changes needed label Apr 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add 'BeautifyMarker' marker type to the project. Rework from Pull ID: #776 #819

Add 'BeautifyMarker' marker type to the project. Rework from Pull ID: #776 #819

Uh oh!

JeremyBYU commented Feb 6, 2018

Uh oh!

Conengmo commented Apr 14, 2018

Uh oh!

JeremyBYU commented Apr 19, 2018

Uh oh!

stickler-ci Apr 25, 2018

Uh oh!

JeremyBYU Apr 25, 2018

Uh oh!

JeremyBYU commented Apr 25, 2018

Uh oh!

Conengmo left a comment

Uh oh!

Conengmo Apr 25, 2018

Uh oh!

Conengmo Apr 25, 2018

Uh oh!

Conengmo Apr 25, 2018

Uh oh!

Conengmo Apr 25, 2018

Uh oh!

Conengmo Apr 25, 2018

Uh oh!

JeremyBYU commented Apr 25, 2018

Uh oh!

ocefpaf commented Apr 25, 2018

Uh oh!

Uh oh!

Add 'BeautifyMarker' marker type to the project. Rework from Pull ID: #776 #819

Add 'BeautifyMarker' marker type to the project. Rework from Pull ID: #776 #819

Uh oh!

Conversation

JeremyBYU commented Feb 6, 2018

Uh oh!

Conengmo commented Apr 14, 2018

Uh oh!

JeremyBYU commented Apr 19, 2018

Uh oh!

stickler-ci Apr 25, 2018

Choose a reason for hiding this comment

Uh oh!

JeremyBYU Apr 25, 2018

Choose a reason for hiding this comment

Uh oh!

JeremyBYU commented Apr 25, 2018

Uh oh!

Conengmo left a comment

Choose a reason for hiding this comment

Uh oh!

Conengmo Apr 25, 2018

Choose a reason for hiding this comment

Uh oh!

Conengmo Apr 25, 2018

Choose a reason for hiding this comment

Uh oh!

Conengmo Apr 25, 2018

Choose a reason for hiding this comment

Uh oh!

Conengmo Apr 25, 2018

Choose a reason for hiding this comment

Uh oh!

Conengmo Apr 25, 2018

Choose a reason for hiding this comment

Uh oh!

JeremyBYU commented Apr 25, 2018

Uh oh!

ocefpaf commented Apr 25, 2018

Uh oh!

Uh oh!