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

Tooltip Template string switch from single quotes to template literal. #955

Merged
merged 1 commit into from
Sep 16, 2018

Conversation

jtbaker
Copy link
Contributor

@jtbaker jtbaker commented Sep 4, 2018

I noticed a bug in the template due to concatenating strings with single quotes which were interrupted with other possessive single quotes. Switching these to template literals should address the issue.
I noticed this with the following string concatenation example from a dataset:

'<div>' + 'City: Saint John's<br>Country: Antigua and Barbuda<br>Continent: North America' + '</div>'

…gle quotes which were interupted with other possessive single quotes. Switching these to template literals should address the issue.

I noticed this with the following string concatenation example from a dataset:

 ``'<div>'
            + 'City: Saint John's<br>Country: Antigua and Barbuda<br>Continent: North America' + '</div>'`
@@ -361,8 +361,8 @@ class Tooltip(MacroElement):
_template = Template(u"""
{% macro script(this, kwargs) %}
{{ this._parent.get_name() }}.bindTooltip(
'<div{% if this.style %} style="{{ this.style }}"{% endif %}>'
+ '{{ this.text }}' + '</div>',
`<div{% if this.style %} style="{{ this.style }}"{% endif %}>`
Copy link
Member

Choose a reason for hiding this comment

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

Interesting. I did not know about template literal. I guess there are more places in folium where that would be needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are good with ES6 syntax right? The template literals in JS are pretty useful - the equivalent of Python 3.6 f-strings and are nice for embedding different types into a string.

@jtbaker jtbaker changed the title Template string switch from single quotes to template literal. Tooltip Template string switch from single quotes to template literal. Sep 5, 2018
@jtbaker jtbaker mentioned this pull request Sep 16, 2018
@ocefpaf ocefpaf merged commit c8a3cd4 into python-visualization:master Sep 16, 2018
@ocefpaf
Copy link
Member

ocefpaf commented Sep 16, 2018

I'll prepare a bugfix release tomorrow. Thanks for the fix!

@Conengmo
Copy link
Member

Here is a page on browser compatibility:
https://caniuse.com/#feat=template-literals

Chrome is since 41, which is from May 2015. Firefox sinds 34 from December 2014. Edge supported it since the start. Internet Explorer doesn't support template literals though.

Maybe there were other incompatibilities with legacy browsers in folium already, but if not we may leave some users behind with this. But I guess there's no excuse for using an outdated browser nowadays.

@Conengmo
Copy link
Member

I've been playing around with this and this is great stuff! Using this in Popup as well fixes multiple issues reported by people who hadn't figured out to use the parse_html argument. I included a list of issues in #850.

Apart from Tooltip and Popup I don't think this is needed in other places. In GeoJsonTooltip you already applied template literals.

@ocefpaf
Copy link
Member

ocefpaf commented Sep 17, 2018

Apart from Tooltip and Popup I don't think this is needed in other places.

Good to know. Thanks for looking into it @Conengmo!

@jtbaker
Copy link
Contributor Author

jtbaker commented Sep 17, 2018

@Conengmo awesome! one question - does this negate the need for parse_html arg in folium.map.Popup? Are there other use cases for it other than being able to interpolate string quotes/apostrophes into the template?

@Conengmo
Copy link
Member

does this negate the need for parse_html arg in folium.map.Popup?

I don't think so. A possible use case if you want to show literal html in your text. (But who would want that? :p) Also, special characters seem to work for me, but there may be cases where you want them converted to html entities.

@jtbaker
Copy link
Contributor Author

jtbaker commented Sep 17, 2018

I think some of the source of the confusion has to do with the naming of the keyword argument. When I read parse_html I read it as "display with html formatting", which anything in the template will do by default since its content writes an HTML document.

I think it would be more clear to end users what it does if it was named something like insert_as_raw_string - or have the logic inverted and name the argument render_as_html and default to True in the constructor.

I'm personally fine with it either way as I don't need it a lot - but I think it might be more clear to people that could need to use it.

@ocefpaf
Copy link
Member

ocefpaf commented Sep 17, 2018

I think it would be more clear to end users what it does if it was named something like insert_as_raw_string - or have the logic inverted and name the argument render_as_html

I like insert_as_raw_string.

and default to True in the constructor.

That was default but it caused other issues so we reverted it.
I need to take a look at the past issues and think about it a little bit.

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