-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Upgrade Leaflet 1.6.0 -> 1.9.3 #1660
Conversation
List of issues: font sizes are very smallThis is related to Leaflet/Leaflet#7800 |
I would try to stay as close to the leaflet defaults as we can. So if there's a change of this sort I'd propagate it and let leaflet to resolve it if that is considered an issue. Though this approach ideally requires folium minor/patch release following leaflet releases. |
Agreed. While I don't like the new font size I don't want to maintain that feature in folium. I'd rather release it more frequently if/when leaflet changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Conengmo I would not re-run all the examples. In a way I want to move towards us never commit the output again. I'm working on a PR that will add them to the docs and run them when building the docs only, in an attempt to keep our repo lighter and the history a bit cleaner.
That sounds good. I'll remove them from the PR after reviewing them. I know we talked about this topic before and one thing I want to stress again is that I think it's important the nbviewer links stay working.
I don't agree. What Leaflet did here is say "we no longer control font size, Leaflet users should". Folium itself is a Leaflet user, more than our users are. So IMO we should take over that control since they dropped it. If we don't do something, I can already see the issues coming in saying folium broke something and how to fix it. Every folium user will have to inject some css with some custom code to fix it, it'll suck. I'd much rather have a reasonable default, have folium 'just work' out of the box. That's what our users expect.
It's one line. We have other CSS as well already. I don't really see this as a maintenance burden: |
My comment here was more general. Like if the default shape of the popup changes, I'd propagate that. If leaflet push something onto user, that is fine. I see folium as a Python API for leaflet and do not expect that it is doing some custom changes of behaviour along the way. That was my point. |
I do appreciate your point, and in general I agree. The more we embrace Leaflet, the easier we have it. I guess I'm a bit confused why Filipe and you argue for this in this case. Leaflet even mentions the need to adjust the font size downstream in their changelog: https://github.com/Leaflet/Leaflet/blob/main/CHANGELOG.md#%EF%B8%8F-breaking-changes-1. So in general I agree, but what about this practical case? |
Those that are not created with permalink will fail :-/
To be honest I missed that bit. I assumed it was a new default. In that case I agree that we should ship something, even though now we "own" that default 🤷. |
I didn't understand this note
as "this needs to be set in downstream". More as "you can if you want". I am fine with this change here as it is proposed right now though. |
Thanks for the input guys! Regarding how to deal with the notebook output, maybe we can take it to a separate issue? I'll also link to the previous discussion we had: #1414 (comment) |
For anyone following along interested in how to change the overall font size in folium after this change:
|
We haven't upgraded our version of Leaflet for a while now. They just released 1.9.3: https://github.com/Leaflet/Leaflet/blob/v1.9.3/CHANGELOG.md
I looked at the breaking changes, especially in 1.8.0, but nothing stood out to me.
I executed all the example notebooks, so we can see whether anything is broken: https://nbviewer.org/github/Conengmo/folium/tree/leaflet-1.9.3/examples/Note that I disabled Codespell on the notebooks for now. I got many warnings, but don't want to fix those in this PR.Looked through a selection of notebooks and didn't spot any regressions related to this change.