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

Popup width #1040

Merged
merged 3 commits into from
Dec 23, 2018
Merged

Popup width #1040

merged 3 commits into from
Dec 23, 2018

Conversation

ocefpaf
Copy link
Member

@ocefpaf ocefpaf commented Dec 11, 2018

@Conengmo this solves problems like #959 (comment)

@ocefpaf ocefpaf requested a review from Conengmo December 11, 2018 19:10
@Conengmo
Copy link
Member

Looks good @ocefpaf. One caveat though is that popups with only text become more narrow than they used to be. For example the 'Fancy HTML popup':

http://nbviewer.jupyter.org/github/ocefpaf/folium/blob/popup_width/examples/Popups.ipynb#Fancy-HTML-popup
http://nbviewer.jupyter.org/github/ocefpaf/folium/blob/master/examples/Popups.ipynb#Fancy-HTML-popup

A solution is that now users of text have to give in a value in pixels. But maybe there's a way to have it work automatically for both graphics and text?

@ocefpaf
Copy link
Member Author

ocefpaf commented Dec 12, 2018

Looks good @ocefpaf. One caveat though is that popups with only text become more narrow than they used to be. For example the 'Fancy HTML popup'

Yep. In a way that is the "default" 100% width reserved for text from leaflet. We can always explicitly request more like in cell number [3].

A solution is that now users of text have to give in a value in pixels. But maybe there's a way to have it work automatically for both graphics and text?

We could try to add some logic to add that behavior. I understand that the behavior in this PR is "kind of a breakage" from the previous one, but now things are more closely related to how leaflet works and less "magic."

I'm not against checking for text vs other object to use two defaults, 300px and 100% respective, but I have to confess I'm not in love with the idea of the extra complexity there.

@Conengmo Conengmo merged commit f8ac8dc into python-visualization:master Dec 23, 2018
@ocefpaf ocefpaf deleted the popup_width branch December 24, 2018 18:02
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.

2 participants