Skip to content

Conversation

@apatil
Copy link
Contributor

@apatil apatil commented Mar 13, 2015

Hi @ocefpaf, another small one. This allows unicode titles on popups.

@ocefpaf
Copy link
Member

ocefpaf commented Mar 13, 2015

Hi @apatil we need to be smarter about this to ensure py3k compatibility.

Also, we will probably need something like:

'fancy string/unicode'.encode('ascii', 'xmlcharrefreplace')

To get a proper HTML representation.

Are you up to implement this? If not, can you open an issue and reference this PR?

folium/folium.py Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use

isinstance(popup, (str, unicode))

instead.

@apatil
Copy link
Contributor Author

apatil commented Mar 13, 2015

Hi @ocefpaf, that's a good test suite. :) I'll give it a shot later today.

@apatil
Copy link
Contributor Author

apatil commented Mar 20, 2015

How does that look, @ocefpaf?

@ocefpaf
Copy link
Member

ocefpaf commented Mar 20, 2015

Looking great @apatil! As a folium maintainer and a non-English speaker Thanks for doing this!!

I will review/test it and merge ASAP.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apatil I am on the fence here. I am not sure if the call to str is necessary, but it does not hurt... What is your argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was getting type errors in python3 without it. The encode('ascii', ...) produces a bytes object, which json.dumps doesn't like.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

str it is then.

@ocefpaf
Copy link
Member

ocefpaf commented Mar 22, 2015

@apatil I just finished review and it is looking really nice. Can you address the my raise request and squash the commits?

Also, can you add an entry in the Changelog.

(If you do not have time for that let me know and I will merge and do it myself 😉 )

@apatil
Copy link
Contributor Author

apatil commented Mar 31, 2015

@ocefpaf sorry for the delay and the extra commit, but it's mostly squashed. :)

ocefpaf added a commit that referenced this pull request Mar 31, 2015
@ocefpaf ocefpaf merged commit 6719db4 into python-visualization:master Mar 31, 2015
@ocefpaf
Copy link
Member

ocefpaf commented Mar 31, 2015

Nice! Thanks for sending another very useful PR!

@apatil
Copy link
Contributor Author

apatil commented Apr 1, 2015

My pleasure. :)

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