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

GeoJson: add to parent after data #1190

Merged
merged 1 commit into from
Jul 29, 2019

Conversation

Conengmo
Copy link
Member

When using GeoJson with non-embedded data, in JS, don't add the object to the parent object until the data has been added.

This should solve the issue where geojson doesn't work with marker_cluster anymore. While not breaking the interaction between geojson and layercontrol.

An alternative is to make the ajax call synchronously, but that doesn't seem like a better approach to me.

@Conengmo
Copy link
Member Author

Here's the code I used to test behavior, using a data file from the folium repo:

with open('examples/data/search_bars_rome.json') as f:
    data = json.load(f)
data_url = 'https://github.com/python-visualization/folium/master/examples/data/search_bars_rome.json'

m = folium.Map((41.9, 12.5), tiles='cartodbpositron')
marker_cluster = folium.plugins.MarkerCluster(name='mc').add_to(m)
folium.GeoJson(data_url, embed=False).add_to(marker_cluster)
folium.GeoJson(data_url, embed=False, name='geojson').add_to(m)
folium.LayerControl(collapsed=False).add_to(m)
m.save('_map.html')

@ocefpaf
Copy link
Member

ocefpaf commented Jul 28, 2019

@Conengmo are you done with the PR or do you want to add a test case to avoid regression? (I'm fine merging this as-is.)

@Conengmo
Copy link
Member Author

@fullonic will have to check first, and discuss.

A test case would be on the JS side, so first we’ll have to setup Selenium tests. I looked into that a bit the other day. Maybe discuss that in another place though.

@fullonic
Copy link
Contributor

@Conengmo I tried your PR and it work's. For me we are good to go with the merge. May be a test and it's it. 👍

@Conengmo
Copy link
Member Author

Thanks for checking it out @fullonic. If this indeed solves your problem without breaking current behavior let's merge it. Would be good to add Selenium tests to our suite before doing another release.

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.

3 participants