-
-
Notifications
You must be signed in to change notification settings - Fork 402
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
make toolbar=True the default in save() #4518
Conversation
This is not true (e.g. you can see the release notes for 1.13.3 here: http://holoviews.org/releases.html), I think we just neglected to mention this in the docs. I also don't think the default should be true as a picture of a toolbar is not useful even if it is useful for docs or other scenarios. I'm sorry about the regression in a micro-release which shouldn't have happened but I'm -1 on reverting to the old behavior. Cc: @jbednar Any opinion? |
Also thanks for the PR, I appreciate the effort even though we disagree on the change. |
See this PR #4519 - if you go to the docs for the |
But it's NOT just for a picture! I use it to serve up the HTML that allows a user to interact with the visualization. The examples that are provided for deploying with Flask and Bokeh use this as well. I think you should keep the OLD default behavior and if someone needs to have it for a PNG, they can turn the toolbar off. |
Oh sorry in that case I'd consider this a bug. The intended behaviour was to only enable this for PNG exports not for static HTML renders. |
So how do you want to handle? Accept this PR as is, and create a new issue relative to making the behavior dependent on the type, and a follow-up PR? Or fix that "bug" in this PR by testing for the 'PNG' format? IMHO, I think that documenting that behavior could be confusing, so you're better off just having the |
As suggested in #4515, I vote for having the toolbar suppressed by default for PNG, SVG, or other static image formats, and enabled by default for "live" formats like HTML. I think that's what nearly everyone will want, so that not many people will ever need to end up reading the docs for how to enable the opposite behavior. |
So I could modify this PR to have that behavior, and then should we remove the
|
That seems good, I'd go with something like this for the condition: if not toolbar and backend == 'bokeh' and (fmt == 'png' or (isinstance(filename, str) and filename.endswith('png'))):
obj = obj.opts(toolbar=None, backend='bokeh', clone=True) |
This also seems like a fairly annoying regression after you've pointed out that this indeed affects all static |
Thanks for being patient with me despite being confused about the actual issue @Dr-Irv and thanks for the PR! |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Reference issue #4515 . Making the toolbar default to
True
to correspond to past behavior.Documentation issue is that the docs for 1.13.3 are not published on the web site, so presumably if they were to get rebuilt the option would show up there.