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

Refactor hardcoded JS CDN links to _protected variables #1312

Merged
merged 6 commits into from
May 10, 2020
Merged

Refactor hardcoded JS CDN links to _protected variables #1312

merged 6 commits into from
May 10, 2020

Conversation

and-viceversa
Copy link
Contributor

folium.py exposes _default_js and _default_css at a high level. This is good, because you can override the CDN URL to point elsewhere. In my case, to point to a static assets folder.

The plugins do not do this. This PR moves the URLs out of their respective render() methods to expose them in a similar way. This shouldn't change any existing behavior.

Also, 98% of the changes are fixing flake8 results. Sorry about that but I went by the contributing guideline.

@Conengmo
Copy link
Member

Conengmo commented Apr 27, 2020

Hi @and-viceversa, thanks for your PR, I think this is a good idea!

For anyone reading along, this was discussed in #351 and more recently in #1312.

Two things:

I'd prefer if each module had the same variable name for the JS and css links. Since we already have _default_js and _default_css in folium.py we can use those names. And also have them all be the same type, a list of str.

Can you remove the style changes in code you otherwise wouldn't touch? It makes reviewing a lot more work, and since I'm doing this in my free time that's something to look out for. I'll update the contributing guide to reflect this as well.

@and-viceversa
Copy link
Contributor Author

and-viceversa commented Apr 27, 2020

Please see updated PR.

The plugins that use them now follow the _default_js and _default_css pattern.

The render() methods use the same for loop as folium.py.

Most plugins only use 1 JS or CSS file, so this is a bit overkill. But now it's uniform.

Thanks for your quick feedback. Let me know what else I can do on this.

Edit: Also, if the PR looks like Git spaghetti - first time doing this not on my own repos. Please advise if that part needs to be cleaner.

@Conengmo
Copy link
Member

Looks good @and-viceversa ! I'll check in more detail before merging, but at this time there's nothing else for you to do. Don't worry about the commits, we squash and merge.

@Conengmo Conengmo added the waiting for review PR is waiting to be reviewed label Apr 27, 2020
folium/plugins/timestamped_geo_json.py Outdated Show resolved Hide resolved
@Conengmo Conengmo removed the waiting for review PR is waiting to be reviewed label May 7, 2020
@Conengmo Conengmo merged commit 3c48be0 into python-visualization:master May 10, 2020
@Conengmo
Copy link
Member

Thanks for your work on this @and-viceversa !

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