Skip to content

Add Locate Control plugin #1116

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

Merged
merged 11 commits into from
Apr 14, 2019
Merged

Conversation

fullonic
Copy link
Contributor

@fullonic fullonic commented Apr 3, 2019

Plugin based on Leaflet Locate Control

Related with issue #1098

@fullonic fullonic force-pushed the locate_control branch 2 times, most recently from 60c0016 to 177ba5b Compare April 3, 2019 14:06
@Conengmo Conengmo added the waiting for review PR is waiting to be reviewed label Apr 4, 2019
@Conengmo
Copy link
Member

Conengmo commented Apr 4, 2019

Hi @fullonic, thanks for your work on this!

Question: the 'files changed' tab shows a lot of unrelated changes. Can you try to rebase or merge master into your branch so we can see a clean diff?

@fullonic
Copy link
Contributor Author

fullonic commented Apr 5, 2019

hey @Conengmo, thanks for your feedback. I just did a rebase as you suggest. I tried to reduce the history of the branch and I end up creating a dirty commit. Now looks better.

I can't understand well why isn't passing all tests in Travis, but I think is not because this PR.

@Conengmo
Copy link
Member

Conengmo commented Apr 5, 2019

Thanks for updating the branch, looks good now. I'll have a look later this weekend.

@fullonic
Copy link
Contributor Author

fullonic commented Apr 5, 2019

Thanks @Conengmo.
Doing some tests I detected a small issue with the Draw plugin. There is a extra ' " ' on line 79

'download', {{ this.filename|tojson }}"

The issue isn't on the last release but on the master branch version.
I fixed in my local repo, shall I push the new commit to this PR?

@Conengmo
Copy link
Member

Conengmo commented Apr 5, 2019

Good find! Yes if you can open a pr for that, great!

@Conengmo Conengmo added waiting for changes This PR has been reviewed and changes are needed before merging and removed waiting for review PR is waiting to be reviewed labels Apr 7, 2019
@Conengmo
Copy link
Member

Conengmo commented Apr 8, 2019

Hi @fullonic, I reviewed your PR yesterday and overal it looks fine. I left mainly comments about style, to get it consistent with the rest of the code base.

Two further questions:

Can you update your branch from master? We got a fix in it that makes the tests work on Travis again.

Could you add a cell to the plugins example notebook? That notebook is a gallery of all the plugins folium has, so would be nice to show this one as well! A very simple example is enough, with one or two lines to explain what it does.

@fullonic fullonic force-pushed the locate_control branch 2 times, most recently from 8eaea53 to 84f73e6 Compare April 9, 2019 07:38
@fullonic
Copy link
Contributor Author

fullonic commented Apr 9, 2019

Hey @Conengmo, I just added right now an example to the notebook. Let me know if it's okay or should I add more information. Thank's for your help.

@Conengmo
Copy link
Member

Conengmo commented Apr 9, 2019

@fullonic looks good! There is a tiny typo in the notebook though: 'displyed'.

Question: why or how does this plugin interfere with Draw? Can we do something about that?

@fullonic
Copy link
Contributor Author

fullonic commented Apr 9, 2019

@fullonic looks good! There is a tiny typo in the notebook though: 'displyed'.

Question: why or how does this plugin interfere with Draw? Can we do something about that?

@Conengmo, the typo is corrected. Thanks

I have no idea why that happen. Even if I change the position of the locate button to "bottomright", for instance, it simple doesn't shows up. Only when changing the order... I didn't dig to much into the conflict because it was easy to "solve".

@Conengmo Conengmo merged commit 8516083 into python-visualization:master Apr 14, 2019
@Conengmo Conengmo removed the waiting for changes This PR has been reviewed and changes are needed before merging label Apr 14, 2019
@Conengmo
Copy link
Member

Thanks @fullonic! It's a nice plugin and the code is nicely done. If somebody wants to figure out why it conflicts with Draw that's welcome, but for now your warning is good enough.

@fullonic
Copy link
Contributor Author

Thanks @Conengmo for your support and help. Was nice to make a contribution to folium!

@fullonic fullonic deleted the locate_control branch June 25, 2019 08:16
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.

2 participants