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

Upgrade to Leaflet v1.x #453

Merged
merged 43 commits into from
Mar 29, 2018
Merged

Upgrade to Leaflet v1.x #453

merged 43 commits into from
Mar 29, 2018

Conversation

jcheng5
Copy link
Member

@jcheng5 jcheng5 commented Aug 18, 2017

(Note: @timelyportfolio actually did all the heavy lifting for this PR, I just created the PR itself)

Remaining work items. I would much appreciate any help with these.

  • Merge recent changes from master into v1.x
  • Upgrade from Leaflet 1.0.3 to 1.2.0
  • Update plugins
  • Go through the various option defaults in Leaflet 1.x docs, and make sure they match our xxxOptions functions and the default values for addXXX functions. (A difference in zIndex defaults broke rasters)
  • Update package version, NEWS
  • Final round of testing

@bhaskarvk can you also make sure that your leaflet.extras don't have any zIndex = NULL either?

timelyportfolio and others added 8 commits March 14, 2017 09:48
The default zIndex for TileLayer and GridLayer is now 1. In
Leaflet 0.7 it was null. In our R code, the default was NULL,
which we pass to the Leaflet code. Therefore, in Leaflet 1.x
we actually override the default zIndex of 1 and change it to
"auto".

However, for addRaster, we've never allowed the zIndex to be
set, so we don't override the new default zIndex of 1. So we
end up with a tile layer with zIndex "auto" and a grid layer
with a zIndex of 1. For "stacking context" reasons that I do
not understand, auto always renders in front of non-auto.

Setting the default for tiles to 1 fixes the problem.
The getImageData(callback) function as previously written
would invoke the callback immediately if image data was
already loaded, or asynchronously if image data was not.
(I knew this was weird so I noted this in the function's
comment.) This turns out to be a problem with Leaflet 1.x
because it expects createTile's callback (done) function
to only be invoked after createTile has returned. If done
is called too soon, then it's ignored, because done first
checks the GridLayer's tile cache to see if the tile that's
done is even still wanted. Since createTile hasn't returned
the tile isn't in the cache yet, and done becomes a no-op.
@jcheng5 jcheng5 mentioned this pull request Aug 18, 2017
@timelyportfolio
Copy link
Contributor

@jcheng5 thanks so much for putting this together. I am excited to get Leaflet (R) > 1.0, since the cadence for Leaflet (JS) is so much quicker now. I'll test and help in any way I can. Let me know if you want me to tackle anything specifically.

@bhaskarvk
Copy link
Collaborator

For leaflet.extras I will need to go thru each and every plugin and make sure it's up to date and works with 1.x. So I need some time. My suggestion is to keep this Github only for now, until I make leaflet.extras ready an do a simultaneous release in say 2 or 3 weeks from now.
How does that sound ?

@bhaskarvk
Copy link
Collaborator

Also for the popups, I was thinking may be we should use https://github.com/yafred/leaflet-responsive-popup instead of the default popups coz responsive popups can be very handy.

@bhaskarvk
Copy link
Collaborator

@jcheng5 One more thing, it's been some while after @timelyportfolio updated the plugins and now. So it might be worth the efforts to update them one more time and also update leaflet to 1.2.0 instead of 1.0.3

@jcheng5
Copy link
Member Author

jcheng5 commented Aug 21, 2017

I've merged from master, and updated to Leaflet 1.2.0. @timelyportfolio would you have time to upgrade the plugins to their latest versions?

@timelyportfolio
Copy link
Contributor

timelyportfolio commented Aug 23, 2017

I will start by assembling a checklist of all the plugins we use. I think I have one somewhere and hope to start chipping away at the list. Are we talking about both leaflet and leaflet-extras?

  • leaflet-markercluster update markercluster->v0.1.1, leaflet-measure->2.1.7, leaflet-omnivore->0.3.4 #458
  • [x ] leaflet-omnivore - doesn't seem to work, but never used before so could easily be me [Update: addGeoJSON seems to be working]
  • leaflet-measure - updated to 2.1.7 and tested
  • Proj4Leaflet
  • Leaflet-MiniMap
  • Leaflet.EasyButton
  • Leaflet.Graticule
  • Leaflet.SimpleGraticule - nothing changed but examples still work
  • Leaflet.Terminator - nothing changed but examples still work
  • Leaflet.awesome-markers - not touched based on @bhaskarvk warning [Update verified working by Bhaskar]
  • leaflet-locationfilter - seems to be broken stuck in 4 year old state [Update: Fixed by Bhaskar]
  • leaflet-providers-plugin - not sure what to do here [Update: I've just upgraded to latest version]

For additional reference, here is the Rmd I used to track errors https://github.com/timelyportfolio/leaflet/blob/v1.0/inst/errors/errors.Rmd.

@bhaskarvk
Copy link
Collaborator

@timelyportfolio just leaflet, I'll deal with the leaflet.extras pkg.

@bhaskarvk
Copy link
Collaborator

bhaskarvk commented Aug 23, 2017

Oh BTW,

awesome markers

leaflet-awesome-markers' original author had long abandoned the project, so I forked it and updated it with some good PRs that were pending on the original repo
https://github.com/bhaskarvk/Leaflet.awesome-markers/commits/2.0/develop
And used this in our leaflet package.

I even submitted a PR to the original author of the plugin but he's MIA. Someone else has taken stewardship of the project and made some changes recently and has a PR pending that is essentially my commits copied over.

sigma-geosistemas/Leaflet.awesome-markers#8

So for awesome-markers I have 2 suggestions.
a) Keep it as it is (i.e. copies from my fork) and just make sure it works correctly under leaflet 1.2.0. If something is not working and needs fixing, fork my repo and fix it and use it.
b) fork the /sigma-geosistemas/Leaflet.awesome-markers repo, merge that pending PR8 and also make sure it works for 1.2.0

either way Don't use the original repo.

Leaflet.Label to L.Tooltip

I had also forked Leaflet.Label and merged some pending PRs https://github.com/bhaskarvk/Leaflet.label/commits/master and used that in our package.

Unfortunately this is going to be a bit tricky because this plugin exists no more and Leaflet.Label has become Leaflet.Tooltip in the original leafletjs. So it might not be possible to support the new additions that I had made in Leaflet.Label in the next iteration of our package, which I am sure will annoy @jcheng5 :).
For now at least compare my changes to Leaflet.Label and see if the L.Tooltip supports them, and if not remove those options from the labelOptions function. Or if you don't want to break API compatibility deprecate them and make then noop (Issue a warning saying they aren't supported or something).

@timelyportfolio
Copy link
Contributor

timelyportfolio commented Aug 30, 2017

@jcheng5 @bhaskarvk not sure how best to track the plugin updates. #458 updates markercluster, and everything seems to work except for freezeAtZoom. I am expecting behavior like http://ghybs.github.io/Leaflet.MarkerCluster.Freezable/examples/mcg-freezable-leaflet1.0.0.html.

@bhaskarvk
Copy link
Collaborator

@timelyportfolio If you fix the freeze support in #458 I will merge it w/ v1.0 branch.

@timelyportfolio
Copy link
Contributor

freeze support fixed in #458.

@bhaskarvk
Copy link
Collaborator

Almost ready https://github.com/bhaskarvk/leaflet/tree/upgrade/leaflet-v1.x,
I've merged #453, #458, upgraded some more plugins, verified all examples.
Some minor issues to be ironed out, once I do that I'll give you a PR.

@bhaskarvk
Copy link
Collaborator

Superseded by #476, but keeping open for tracking purposes.

@schloerke schloerke mentioned this pull request Feb 12, 2018
29 tasks
@schloerke schloerke merged commit d19bd52 into master Mar 29, 2018
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.

4 participants