-
Notifications
You must be signed in to change notification settings - Fork 476
Add a default toolbar for figure #1394
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
Conversation
martinRenou
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I only have minor comments
bqplot/pyplot.py
Outdated
|
|
||
|
|
||
| def show(key=None, display_toolbar=True): | ||
| def show(key=None, display_toolbar=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we really change this? This will break long-time users assumption that it shows a toolbar by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, maybe display_toolbar should be kept True by default and used for setting the figure's show_toolbar property?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking again about this, this sounds like a good idea by @martinRenou.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove the old and ugly toolbar in this PR as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we should since user can still add this ugly toolbar independently.
|
Added animation for |
martinRenou
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Would you mind squashing the commits a bit?
|
Thanks @martinRenou, yes can you squash it please! |
|
Note: I think this is a 0.13 thing since it is a significant change of behavior. |
|
Should we move forward introducing bqscales and removing threejs then? I was also wondering if we should still maintain a 0.12.x branch? |
|
Yes, we should start merging thing and create a 0.12.x branch pointing at the last commit before this PR. |
|
@maartenbreddels just wanted to ping you here. You should be interested in this. |
|
Thanks for the ping. |
References
This patch adds a default toolbar to
Figurewidget, this toolbar contains the same buttons as inToolbarwidget but it is implemented directly inFigureclassUser-facing changes
display_toolbar(defaultTrue) is added to constructor ofFigureto show or hide the toolbar.pyplotfigure. Nowdisplay_toolbarparameter ofplt.showis used to show or hide the default toolbar.