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

update markercluster->v0.1.1, leaflet-measure->2.1.7, leaflet-omnivore->0.3.4 #458

Closed
wants to merge 35 commits into from

Conversation

timelyportfolio
Copy link
Contributor

@timelyportfolio timelyportfolio commented Aug 30, 2017

I updated

  • leaflet-markerCluster - All seems to work except for freezeAtZoom using example
  • leaflet-measure - think all good
  • leaflet-omnivore - tried and doesn't seem to work
    topoData <- readLines("https://d3js.org/us-10m.v1.json") %>% paste(collapse = "\n")
    
    leaflet() %>% setView(lng = -98.583, lat = 39.833, zoom = 3) %>%
      addTiles() %>%
      addTopoJSON(topoData, weight = 1, color = "#444444", fill = FALSE)
    

@timelyportfolio timelyportfolio mentioned this pull request Aug 30, 2017
6 tasks
@timelyportfolio timelyportfolio changed the title update of markercluster to v0.1.1 update markercluster to v0.1.1 and leaflet-measure to 2.1.7 Aug 30, 2017
@timelyportfolio timelyportfolio changed the title update markercluster to v0.1.1 and leaflet-measure to 2.1.7 update markercluster->v0.1.1 and leaflet-measure->2.1.7 Aug 30, 2017
@timelyportfolio timelyportfolio changed the title update markercluster->v0.1.1 and leaflet-measure->2.1.7 update markercluster->v0.1.1, leaflet-measure->2.1.7, leaflet-omnivore->0.3.4 Aug 30, 2017
@bhaskarvk
Copy link
Collaborator

There were 2 sub plugins of the marker cluster plugin.
https://github.com/ghybs/Leaflet.MarkerCluster.LayerSupport To support Layer Group Control + Clustering &
https://github.com/ghybs/Leaflet.MarkerCluster.Freezable To support Freezing.
You probably need to upgrade them both too.

@timelyportfolio
Copy link
Contributor Author

@bhaskarvk, I updated both this morning, and I don't see any API changes, but for some reason, freezeAtZoom is not working. Here is the freeze example from the plugin where it is working with Leaflet 1.0.1

I'll keep trying.

@timelyportfolio
Copy link
Contributor Author

timelyportfolio commented Aug 31, 2017

@bhaskarvk, I just figured it out. freeze and layersupport don't work together. If I use markerClusterGroup() instead of markerClusterGroup.layerSupport, then freezeAtZoom works correctly. I will try to debug to submit pull or at a minimum file a detailed issue.

Filed ghybs/Leaflet.MarkerCluster.LayerSupport#13.

@timelyportfolio
Copy link
Contributor Author

... but (maybe @bhaskarvk can help) I just discovered that the easyButton example freeze works. This could be that it uses markerCluster directly instead of layersupport.

@bhaskarvk
Copy link
Collaborator

@timelyportfolio If I have to chose between freeze support or layersupport I would chose layersupport.. The layersupport plugin is crucial for supporting clustering + layers. I think that is more important to have than freezeAtZoom.
Also in 0.7.x the freezeAtZoom + layerSupport was working perfectly fine, so it's probably best to raise an issue w/ these plugin author/s to see why it's not working in 1.2.0.

@timelyportfolio
Copy link
Contributor Author

@bhaskarvk I got a very quick response from the author with a workaround (ghybs/Leaflet.MarkerCluster.LayerSupport#13), but I'm not sure we will want to include. I tend to agree layer support is more important than freezable.

@bhaskarvk
Copy link
Collaborator

@timelyportfolio Why are you unsure about including the fix ?

@bhaskarvk
Copy link
Collaborator

I am fine with using a fixed plugin. I have had to do that in past too for some of the plugins in both leaflet and leaflet.extras. @timelyportfolio could you add the fix to this PR and I can test it out.

@timelyportfolio
Copy link
Contributor Author

Thanks @bhaskarvk I hope to get this done by end of the weekend.

@timelyportfolio
Copy link
Contributor Author

timelyportfolio commented Sep 29, 2017

@bhaskarvk, I finally got it to work using ghybs/Leaflet.MarkerCluster.Freezable@d395303 and changing the order of the load/dependencies commit.

Please test if you get a chance.

I believe this leaves only leaflet-omnivore, leaflet.awesome-markers, leaflet-locationfilter, leaflet-providers-plugin as unresolved. These are listed in #453 (comment).

@timelyportfolio
Copy link
Contributor Author

@bhaskarvk, have you had a chance to look at this? Thanks.

@bhaskarvk
Copy link
Collaborator

I will look over this over the coming weekend. Promise!

@bhaskarvk bhaskarvk self-assigned this Oct 10, 2017
@bhaskarvk
Copy link
Collaborator

I will review this tonight and merge in the 1.x branch

@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

I've merged this in my https://github.com/bhaskarvk/leaflet/commits/upgrade/leaflet-v1.x and will be pushed as part of LeafletJS 1.x upgrade.

@bhaskarvk bhaskarvk mentioned this pull request Nov 13, 2017
@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
Copy link
Contributor

This code has been merged into master #492. Closing. Thank you!!

@schloerke schloerke closed this Mar 30, 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.

7 participants