Skip to content

Conversation

@jelmelk
Copy link
Contributor

@jelmelk jelmelk commented Nov 27, 2015

This is to address #259. Assuming I did the PR properly.

@ocefpaf
Copy link
Member

ocefpaf commented Nov 27, 2015

Awesome @jelmelk! Thanks for the PR!

Can you build a test for this behavior?

@jelmelk
Copy link
Contributor Author

jelmelk commented Nov 27, 2015

Am I correct to assume you're talking about test_folium.py?

@ocefpaf
Copy link
Member

ocefpaf commented Nov 27, 2015

You are adding this behavior for Marker, CircleMarker, and RegularPolygonMarker. Correct?

I guess we should also add this for lines...

@ocefpaf
Copy link
Member

ocefpaf commented Nov 27, 2015

Am I correct to assume you're talking about test_folium.py?

Yep.

@jelmelk
Copy link
Contributor Author

jelmelk commented Nov 27, 2015

You are adding this behavior for Marker, CircleMarker, and RegularPolygonMarker. Correct?

Yes, those are the only ones that needed it. So would it be a single function for each one?

@ocefpaf
Copy link
Member

ocefpaf commented Nov 27, 2015

Yes, those are the only ones that needed it. So would it be a single function for each one?

Maybe we could have been more clever and try to avoid that repetition, but your implementation is OK. I will take a better look tomorrow (going to bed now).

@BibMartin any observations?

@jelmelk
Copy link
Contributor Author

jelmelk commented Nov 27, 2015

I agree, but like I said in #259 (comment) it could be done individually or in one spot that catches all three. Let me know tomorrow, I can change it if needed.

@BibMartin
Copy link
Contributor

Hi @jelmelk ; thanks for the PR !
Provided the Marker class is very simple, I think you can implement it only in Marker and let the other classes inherit from it.
BTW it would make sens to have all marker stuff inherit from the same root class.
Anyway, we're only sparing 6 lines of code this way ➡️ it's not strategic and I would accept the PR like this.

@BibMartin BibMartin mentioned this pull request Dec 2, 2015
@ocefpaf ocefpaf added this to the v0.2.0 milestone Dec 3, 2015
@jelmelk
Copy link
Contributor Author

jelmelk commented Dec 4, 2015

Due to the branch mix up on my part, I'm closing this PR.

@jelmelk jelmelk closed this Dec 4, 2015
@ocefpaf ocefpaf added the enhancement Feature request or idea about how to make folium better label Feb 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Feature request or idea about how to make folium better

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants