Allow popups to be open on page load #778

jwhendy · 2017-11-25T01:45:56Z

This is a first stab at #210 , but should probably have some input from the community.

Currently, this creates a new argument, folium.Popup(..., default_open=False) which utilizes autoClose option from L.popup. This was found on this SO question in which a couple answers cited autoClose: false.

This issue is also enlightening, as there are some other nuanced arguments that control the behavior we might want to consider in folium. For example, the autoClose docs say:

Set it to false if you want to override the default behavior of the popup closing when another popup is opened.

However, the popup will still close when you click another popup because you're also considered to be clicking on the map itself. Currently this PR assumes that if you want them to be open on load, you don't want them to close when clicking another, so closeOnClick is also applied if default_open=True is used.

Lastly, there's an argument to L.map() that could also be relevant, closePopupOnClick.

Some items I think should be reviewed:

the variable name, default_open
is the current approach sane with the inserted if statement (e.g. {% if this.default_open %}, autoClose: false, closeOnClick: false{% endif %}});)?
should other variables be added in order to allow users more granularity, giving access to autoClose, closeOnClick, and closePopupOnClick?
is folium.Popup the right place to add this, or does it make sense any higher up in the chain like folium.Marker or folium.Map (leaning toward it being correct as it is, which maps leaflet options directly to their folium analogs).

Anything else? I'm newer to python and know nearly zilch of javascript, so comments/feedback are welcome!

ocefpaf · 2017-11-25T12:18:20Z

I'm newer to python and know nearly zilch of javascript, so comments/feedback are welcome!

Everything looks good to go! Thanks!! Can you:

add a note on the docstring about the new option;
add an entry to the changelog file;
add a test to ensure future changes won't break this.

Regarding your questions:

the variable name, default_open

I guess we can keep it simple and just name it open.

is the current approach sane with the inserted if statement (e.g. {% if this.default_open %}, autoClose: false, closeOnClick: false{% endif %}});)?

I like it. Simple and "hackable." I don't really want to wrap everything leaflet can do with popups b/c I am not sure the complexity is worth it. However, this extra keyword makes sense to me.

should other variables be added in order to allow users more granularity, giving access to autoClose, closeOnClick, and closePopupOnClick?

This is what I am on the fence. I don't really want to make it overly complex. open by default is very useful, the other not so much. I can be convinced otherwise but I don't think it is worth investing in that.

is 'folium.Popupthe right place to add this, or does it make sense any higher up in the chain likefolium.Markerorfolium.Map(leaning toward it being correct as it is, which mapsleafletoptions directly to theirfolium` analogs).

I may be wrong but Popup does not inherit from Maker, so we should either re-factor to fix that or we should implement this functionality on both. Not sure about folium.Map, the init class there is already overloaded with options.

jwhendy · 2017-11-25T17:32:39Z

@ocefpaf thanks for the feedback!

default_open is now open (does it bother you that open triggers syntax highlighting since it's a built-in?)
docstring added:

    open: bool, default False
        True renders the popup open on page load

wrestling with what the test should be. I've only done one test before and it was checking function output; not sure what it means in this case? Would I just be making sure that I get this if open=True is passed?

var name = L.popup({maxWidth: 'max_width'
    , autoClose: false, closeOnClick: false});

I think what you're getting after is that if someone changes the template, maybe my if stuff gets mucked up. I'm not sure on a flexible way to check, though. Maybe someone puts an option between max_width and open; I'm trying to think of a flexible way not to break the test in that case.

This is what I am on the fence. I don't really want to make it overly complex. open by default is very useful, the other not so much. I can be convinced otherwise but I don't think it is worth investing in that.

Sounds good to me, and I think closePopupOnClick from L.map is not that useful. It's just static labels that can't be closed. So, it's down to the other two.

Can you confirm, then, that we should just bring in closeOnClick: false with the choice to render open on page load? You can compare the behavior of these to see the effect:
- this one only uses autoClose. Click both pins to close them. Now click the left, then the right, back and forth. Each time you open the left, the right will close. After the initial load in which they're open, I think the behavior is undesirable.
- this one works, in my opinion, how people would expect. Popups only close when they themselves clicked, not when anything else happens.

Once we confirm on the few remaining items above, I'll re-push a [hopefully] final commit!

ocefpaf · 2017-11-27T13:55:11Z

default_open is now open (does it bother you that open triggers syntax highlighting since it's a built-in?)

Yes it does. Sorry. We should not shadow builtin and I an terrible with names. Maybe we should name it toggle?

wrestling with what the test should be. I've only done one test before and it was checking function output; not sure what it means in this case? Would I just be making sure that I get this if open=True is passed?

Yep. Unfortunately that is not much we can do beyond that. Still that kind of test help a lot when adding other features and ensuring that we don't break this one.

I'll get to the rest of your PR and message later. I have to go back to the day job now 😬

…on-visualization#771)

jwhendy · 2017-11-27T15:34:19Z

Taking a break from my day job :)

Maybe we should name it toggle

I thought about open_on_load initially, which actually struck me as pretty straightforward/explanatory. toggle is also pretty reasonable; it just doesn't convey that we're toggling them all open. render_open, make_open, show_open, or maybe just show? I actually like that... show. Passing true means show it by default.

Still that kind of test help a lot when adding other features and ensuring that we don't break this one.

Sounds good. I'm a little swamped now that the weekend is over and I have to actually do work again. I'm probably good on input until I figure out how to write a test (and I'll just look at some of the other tests for ideas). Thanks!

ocefpaf · 2017-11-27T16:53:14Z

I actually like that... show. Passing true means show it by default.

show it is 😄

Sounds good. I'm a little swamped now that the weekend is over and I have to actually do work again. I'm probably good on input until I figure out how to write a test (and I'll just look at some of the other tests for ideas). Thanks!

No problem. I won't have time to review it until the end of the year 😬

…n#791) * Set min_zoom default to Leaflet value (0) * Change min_zoom in test

jwhendy · 2017-12-10T18:19:46Z

Update. Per discussion on #784 , I rather like the new idea that's come up to split the open on load and clicking behavior into separate options. I also like the names. The proposal:

show: autoClose and openPopup(). Popups will start open on page load, but toggling one of them, or simply clicking the map will make them go away. That'd be this behavior
sticky: autoClose and closeOnClick. Clicking the map or other popups will not close the popup. That would be this behavior

I just made these changes and things seem to work nicely. Here's a jupyter notebook I used to run through the combos (to try you'll obviously need to install my branch).

* Added max_native_zoom keyword

List both 0.5.0 and master docs

Set overlays untoggled in LayerControl

Fix: max_native_zoom in test

Conengmo · 2018-04-14T12:25:56Z

Hi @jwhendy, I want to continue on working to merge this PR. I think it's great, really simple and effective. Do you want to help with two final things?

Add a line to the changelog
Write a test. I think you already thought about this before. It's okay if it's 'just' a rigid check if the option got added, most folium tests are that way.

jwhendy · 2018-05-12T22:36:51Z

Pretty sure I'm rebased correctly on top of master, so that's one part resolved.

I'm still feeling quite dense on this test thing... from your example code, what would rendered be expected to contain? I get an empty string. I was assuming I'd get the raw js that gets injected into the page and then manually enter in what that should be.

from folium.map import Popup, FeatureGroup

def test_popup_sticky():
    fg = FeatureGroup()
    popup = Popup('Some text.', sticky=True).add_to(fg)
    rendered = popup._template.render(this=popup, kwargs={})
    expected = u"""some javascript with the same name inserted""".format(popup.get_name())
    return rendered
    
test_popup_sticky()
>>> ''

Conengmo · 2018-05-13T15:19:41Z

Good that you solved the conflicts. I'm assuming you will push that commit later.

Weird that you're getting an empty string. Maybe you can try using another test as an example, e.g. test_circle(). I made another snippet that works for me:

m = folium.Map()
marker = folium.Marker([0, 0]).add_to(m)
popup = Popup('Some text.', sticky=True).add_to(marker)
rendered = marker._template.module.script(marker)

Note that we can accept this pull request without a test, so if it's too much trouble we can leave it out. But of course it would be good to figure this out.

…d changelog

stickler-ci · 2018-05-13T17:47:35Z

folium/map.py

+            {% endfor %}
+        """)  # noqa
+
+


W293 blank line contains whitespace

stickler-ci · 2018-05-13T17:47:35Z

folium/map.py

+        """)  # noqa
+
+
+    def __init__(self, html=None, parse_html=False, max_width=300, show=False, sticky=False):


E303 too many blank lines (2)

stickler-ci · 2018-05-13T17:47:35Z

tests/test_map.py

+            , autoClose: false
+            , closeOnClick: false}});
+
+


W293 blank line contains whitespace

stickler-ci · 2018-05-13T17:47:35Z

tests/test_map.py

+
+                var {1} = $(\'<div id="{1}" style="width: 100.0%; height: 100.0%;">Some text.</div>\')[0];
+                {0}.setContent({1});
+


W293 blank line contains whitespace

stickler-ci · 2018-05-13T17:47:35Z

tests/test_map.py

+
+            {2}.bindPopup({0});
+
+


W293 blank line contains whitespace

stickler-ci · 2018-05-13T17:47:36Z

tests/test_map.py

+
+    assert rendered == expected
+
+


W293 blank line contains whitespace

stickler-ci · 2018-05-13T17:47:36Z

tests/test_map.py

+            , autoClose: false
+            }});
+
+


W293 blank line contains whitespace

stickler-ci · 2018-05-13T17:47:36Z

tests/test_map.py

+
+                var {1} = $(\'<div id="{1}" style="width: 100.0%; height: 100.0%;">Some text.</div>\')[0];
+                {0}.setContent({1});
+


W293 blank line contains whitespace

stickler-ci · 2018-05-13T17:47:36Z

tests/test_map.py

+
+            {2}.bindPopup({0});
+
+


W293 blank line contains whitespace

stickler-ci · 2018-05-13T17:47:36Z

tests/test_map.py

+
+
+        """.format(popup.get_name(),
+           list(popup.html._children.items())[0][0],


E128 continuation line under-indented for visual indent

stickler-ci · 2018-05-13T17:50:42Z

tests/test_map.py

+
+                var {1} = $(\'<div id="{1}" style="width: 100.0%; height: 100.0%;">Some text.</div>\')[0];
+                {0}.setContent({1});
+


W293 blank line contains whitespace

stickler-ci · 2018-05-13T17:50:42Z

tests/test_map.py

+
+            {2}.bindPopup({0});
+
+


W293 blank line contains whitespace

stickler-ci · 2018-05-13T17:50:43Z

tests/test_map.py

+
+
+        """.format(popup.get_name(),
+           list(popup.html._children.items())[0][0],


E128 continuation line under-indented for visual indent

jwhendy · 2018-05-13T18:09:39Z

I think I'm good, or pretty darn close at this point. Sorry that the git commit history is pretty gross...

fixed conflicts with master
tests added; is there a better way to get at the html id? I'm using list(popup.html._children.items())[0][0]. It's in the rendered javascript, so I needed some way to pass it to into expected
added API change to changelog under 0.6.0. Let me know if that's not the right place.
the stickler thing appears new... I need the whitespace it's complaining about to make the templates work.

Edit: Oh, and I'm near positive I had an issue with the locally installed branch not printing with render(). I bet it had to do with the fact that the update to master moved self._template to _template and I just didn't do something right. Sorry about that; it worked great after reinstalling.

Conengmo · 2018-05-14T05:45:05Z

Great, seems like we're almost there. Don't worry about the commit history, we'll squash it.

First of all, it seems that the openPopup() call got lost.

is there a better way to get at the html id?

What are doing seems fine. You can also directly get the keys instead of the items:

list(popup.html._children.keys())[0]

added API change to changelog under 0.6.0

It's a new feature, not an API change that users will have to deal with. So you can add it in the main list.

the stickler thing appears new... I need the whitespace it's complaining about to make the templates work.

Yeah that one's a bit annoying isn't it. We'll have to fix these lint errors though, otherwise the code will become a mess. What you have to do is make sure that there aren't any spaces on your blank lines. You don't have to add another blank line, just remove the spaces from it. For example in map.py there are now two blank lines before the init of Popup, remove one and make sure there are no spaces on the remaining line.

Finally, I have a suggestion for your tests. Right now the spaces and newlines have to be exactly the same in comparing rendered and expected. You can make it more robust by removing whitespaces and newlines before comparing. You could copy over the _normalize() function from test_vector_layers.py to test_map.py and normalize rendered and expected before comparing.

jwhendy · 2018-05-14T18:45:54Z

@Conengmo All sounds great. I'll have this done tonight.

Appreciate the tips on the test (such a great idea and newlines/spaces killed me trying to get right!) and where to add the changelog bit.

I'm not surprised there were accidental spaces an will fix those. For this:

For example in map.py there are now two blank lines before the init of Popup...

I actually thought defs should be preceded by two blank lines? I just re-educated myself with some PEP-8 action and see that's top level functions and classes... within a class it's just one. Oops!

jwhendy · 2018-05-15T02:15:05Z

I think we're good now!

I modified the test slightly, using ''.join(rendered.split()). This is nice as the line breaks don't even matter that way. Hope that's alright.
I updated with named format() args, which was a nice touch I saw in a different test.
change log entry moved, whitespace fixed, and updated to use keys()

Aaannnnd, while I was re-reading your comments for maybe the fourth time... how the heck did I miss this staring me in the face:

First of all, it seems that the openPopup() call got lost.

Indeed it did! I blame rebasing on master but am not sure how! I guess that's pretty important. I've got that back in, updated the _show test to account for it, and also re-ran through all of these tests to make sure the map behavior is as-desired. I think we're actually good now!

Conengmo · 2018-05-15T07:51:16Z

Merged! Congrats @jwhendy and thanks for all your efforts!

Closes #784 and closes #210.

jwhendy · 2018-05-15T14:15:13Z

Woo-hoo! And thanks to you for your patience/coaching! I learned quite a few things.

Conengmo · 2018-05-16T07:57:47Z

Good to hear. And remember you're always welcome to pick up another issue, or contribute by commenting/reviewing other stuff.

ptrk8 · 2018-05-30T09:45:38Z

Hey guys, any idea why I can't seem to use the new properties yet? Is it because it hasn't been released yet?
I'm getting the following error when using "show" as one of the properties in folium.Popup().

folium.CircleMarker([-33.7, 151.1160728],
                    radius = 3,
                    popup = folium.Popup('Some text.', show=True),
                    color = '#3186cc',
                    fill_color = '#3186cc',
                   ).add_to(map)

TypeError: init() got an unexpected keyword argument 'show'

Conengmo · 2018-05-30T16:43:38Z

Indeed it hasn't been released yet to Pypi. So if you're on version 0.5 it won't work. Try installing it from GitHub.

jwhendy added 9 commits November 23, 2017 14:23

test making popup always on

a935b3e

argument for default open; attempt at an if statement

6e49163

extra brace

872e597

trying autoClose option to keep all open

ee4747f

second attempt at autoClose

43f4831

trying if statement on autoClose

165b3f3

missed a brace

7166568

options are one dictionary, not separate

598f73f

added closeOnClick option as well

ed1cfa7

Note issue-726 solution in Marker documentation and add example (pyth…

53bf936

…on-visualization#771)

This was referenced Nov 28, 2017

Untoggle overlays and additional base layers #772

Merged

Sticky Popups #784

Closed

Conengmo and others added 4 commits December 7, 2017 09:51

Set min_zoom default to Leaflet value (0 i.o. 1) (python-visualizatio…

106d829

…n#791) * Set min_zoom default to Leaflet value (0) * Change min_zoom in test

changed option to render on load from default_open to show

cd672b7

added sticky option and split out leaflet options

5c0598c

goofed a curly brace

5cf1699

Conengmo mentioned this pull request Dec 13, 2017

Fancy HTML does not display from inside a for loop #799

Closed

Conengmo and others added 6 commits December 16, 2017 09:48

Tilesets native zoom (python-visualization#792)

b03b81e

* Added max_native_zoom keyword

Added gallery links (python-visualization#794)

ccefca0

Link to stable and newest docs (python-visualization#800)

5739244

List both 0.5.0 and master docs

Untoggle overlays and additional base layers (python-visualization#772)

9e2ee74

Set overlays untoggled in LayerControl

max_native_zoom missing in test

461479e

Merge pull request python-visualization#844 from Conengmo/repair-test

4bdcb85

Fix: max_native_zoom in test

jwhendy added 7 commits May 12, 2018 17:21

trying if statement on autoClose

7e7075e

missed a brace

5ab0009

options are one dictionary, not separate

1f01036

added closeOnClick option as well

e8c37cf

changed option to render on load from default_open to show

c387e33

added sticky option and split out leaflet options

62b4612

goofed a curly brace

927949a

jwhendy added 2 commits May 13, 2018 12:43

merged master updates, added tests for sticky/show popup args, update…

7c98694

…d changelog

remote tip behind. grrr

c58aa5d

stickler-ci reviewed May 13, 2018

View reviewed changes

fixed some whitespace

f0c1935

stickler-ci reviewed May 13, 2018

View reviewed changes

jwhendy added 2 commits May 14, 2018 20:58

moved changelog entry, refined test, fixed whitespace

eaca585

added openPopup() back

a78ed4e

Blank lines in tests

f45005d

Conengmo merged commit fac363e into python-visualization:master May 15, 2018

Conengmo removed the PR: changes needed label May 15, 2018

Conengmo mentioned this pull request May 15, 2018

permanent popups #210

Closed

jwhendy deleted the open-popup branch May 15, 2018 18:28

		""") # noqa


		def __init__(self, html=None, parse_html=False, max_width=300, show=False, sticky=False):


		var {1} = $(\'<div id="{1}" style="width: 100.0%; height: 100.0%;">Some text.</div>\')[0];
		{0}.setContent({1});



		""".format(popup.get_name(),
		list(popup.html._children.items())[0][0],

Allow popups to be open on page load #778

Allow popups to be open on page load #778

Uh oh!

Conversation

jwhendy commented Nov 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ocefpaf commented Nov 25, 2017

Uh oh!

jwhendy commented Nov 25, 2017

Uh oh!

ocefpaf commented Nov 27, 2017

Uh oh!

jwhendy commented Nov 27, 2017

Uh oh!

ocefpaf commented Nov 27, 2017

Uh oh!

jwhendy commented Dec 10, 2017

Uh oh!

Conengmo commented Apr 14, 2018

Uh oh!

jwhendy commented May 12, 2018

Uh oh!

Conengmo commented May 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jwhendy commented May 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Conengmo commented May 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jwhendy commented May 14, 2018

Uh oh!

jwhendy commented May 15, 2018

Uh oh!

Conengmo commented May 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jwhendy commented May 15, 2018

Uh oh!

Conengmo commented May 16, 2018

Uh oh!

ptrk8 commented May 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Conengmo commented May 30, 2018

Uh oh!

Uh oh!

jwhendy commented Nov 25, 2017 •

edited

Loading

jwhendy commented May 13, 2018 •

edited

Loading

Conengmo commented May 14, 2018 •

edited

Loading

Conengmo commented May 15, 2018 •

edited

Loading

ptrk8 commented May 30, 2018 •

edited

Loading