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

Draw export button refactor #1342

Closed
wants to merge 11 commits into from
Closed

Draw export button refactor #1342

wants to merge 11 commits into from

Conversation

and-viceversa
Copy link
Contributor

This PR is similar to the scope change in #1312.

draw.py has an optional export button. As far as I can tell, there is no way to override its hardcoded <style>. Move export_style variable outside the Draw class and add Pythonic protected underscore. This shouldn't change default or expected behavior.

Plugins seem more open to interpretation than the core library ... is there any guidance to avoid this sort of refactor in the first place? To me, this sort of property could also belong to self.

@Conengmo
Copy link
Member

Good that you're taking this on, that export button is weird.

  • I agree the styling for the button should be editable. I don't think it should be a module variable. As you said, putting it in self seems like a better idea to me.
  • Better default styling of the export button would be good. It now has an absolute position, that may fall under the layer control. But note that this aspect is not a requirement for getting this PR merged.

About guidance on plugin development: folium has grown organically over the years with different maintainers, so that may explain the different styles in the code.

@Conengmo Conengmo added the waiting for changes This PR has been reviewed and changes are needed before merging label Jun 14, 2020
@Conengmo
Copy link
Member

Conengmo commented Nov 7, 2022

This PR is quite stale, so I'll close it. If somebody wants to pick this up in a new PR and address the comment about putting the styling in self, that's still welcome.

@Conengmo Conengmo closed this Nov 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for changes This PR has been reviewed and changes are needed before merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants