Upgrade Leaflet 1.6.0 -> 1.9.3

[Conengmo]

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.

[Conengmo]

List of issues:

font sizes are very small

This is related to Leaflet/Leaflet#7800

[martinfleis]

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.

[ocefpaf]

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.

[ocefpaf]

@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.

[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 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

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.

While I don't like the new font size I don't want to maintain that feature in folium.

It's one line. We have other CSS as well already. I don't really see this as a maintenance burden: .leaflet-container { font-size: 1rem; }. I think having to deal with confused users asking how to fix the font size will be much worse.

[martinfleis]

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.

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.

[Conengmo]

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?

[ocefpaf]

I want to stress again is that I think it's important the nbviewer links stay working.

Those that are not created with permalink will fail :-/

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?

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 🤷.

[martinfleis]

I didn't understand this note

Allow fonts to respect users' browser settings by making the font-size relative to the map container. (You can change the font size on leaflet-container to adjust it if needed.)

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.

[Conengmo]

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)

[Conengmo]

For anyone following along interested in how to change the overall font size in folium after this change:

m = Map()
Element(
    '<style>.leaflet-container { font-size: 2rem !important; }</style>'
).add_to(m.get_root().header)