-
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
Add SideBySide plugin #1292
Add SideBySide plugin #1292
Conversation
Hi @Conengmo, I have not seen However, I do not know which are the principles for introducing folium plugins, so I'm not sure if you should reconsider introducing it. |
I defined some criteria for plugin acceptance here: #1080 (comment). I think we're good on all accounts except maybe for the interesting new functionality. But looking back on that previous PR I was maybe a bit too strict. If you would like to add this, then I suppose it's wanted. To merge this, I think we require:
|
@Conengmo I don't see what is wrong with the last commit. It seems a problem with a notebook but I cannot spot the problem. Could you take a look? |
Travis build failing is not related to your change. We actually merged a PR yesterday that fixes it, so if you rebase onto master the tests should pass. But you can also leave it like it is now, that's okay. So is this PR ready for review again? |
Yes, it is ready for a review |
Link to notebook changes: https://nbviewer.jupyter.org/github/fralc/folium/blob/master/examples/Plugins.ipynb It seems the notebook outputs are empty. I have some comments on the controls with this plugin. Adding the SBS layer to the control doesn't seem to do anything. The slider will appear even if you untoggle it. So we might as well remove the By default if you create a
Both is also possible. What are your thoughts on these comments? Two things about the CDN link: I see I mentioned raw.githack.com before, but that is actually the development subdomain. We should use the rawcdn production subdomain instead. And better to use the minified JS version. https://rawcdn.githack.com/digidem/leaflet-side-by-side/6ea4567fe8d1b3e6ea3b0e9ca05d311f5be125a3/leaflet-side-by-side.min.js should do it. Other than that no comments. If we can resolve these than I'm sure we're close to merging this. |
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.
Looks good!
I updated the class to use our JSCSSMixin, which simplifies dependency statements. Also included a CND link using jsdeliver.net.
Since layer control does nothing for this plugin, I removed those arguments and hard-coded the plugin to not appear in layer control.
Let's merge this when the tests pass.
I think it would be great to have an option that allows users to keep the layers control. This is particularly useful when users want to display multiple layers on the map when the two layers for the layer control do not cover the entire area. |
Here are some user cases where the layers control would still be useful. |
I think that's still possible. Removing the plugin from layer control is only about the sideBySideLayer plugin itself, which is not a layer, and doesn't do anything in layer control |
In the first example on the page you shared, I'm talking about the 'Split Control' checkbox which does nothing. |
Sorry for my misunderstanding. That makes sense. The Split Control itself should not show up as a layer in the layers control It should be removed. |
No worries, thanks for looking out! |
For anybody checking this out in the future, here's an example of having a base layer and two overlays that can be slided.
|
Looking forward to the new release. Thank you! |
and thanks to @fralc for the PR! Sorry it took so long. |
Sorry for not helping with the final steps, thank you for completing this work! |
Basic parsing of the leaflet-side-by-side plugin following other plugins implementation