Skip to content

Conversation

@trungleduc
Copy link
Contributor

@trungleduc trungleduc commented Aug 6, 2021

References

This patch adds a default toolbar to Figure widget, this toolbar contains the same buttons as in Toolbar widget but it is implemented directly in Figure class

User-facing changes

  • A new toolbar is shown on top right of figure when the cursor is over the figure.

bq1

  • New boolean parameter display_toolbar (default True) is added to constructor of Figure to show or hide the toolbar.
  • The toolbar widget is removed from pyplot figure. Now display_toolbar parameter of plt.show is used to show or hide the default toolbar.

bq2

Copy link
Member

@martinRenou martinRenou left a 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):
Copy link
Member

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.

Copy link
Member

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?

Copy link
Member

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.

Copy link
Member

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?

Copy link
Contributor Author

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.

@trungleduc
Copy link
Contributor Author

Added animation for pyplot.show behavior.

Copy link
Member

@martinRenou martinRenou left a 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?

@trungleduc
Copy link
Contributor Author

Thanks @martinRenou, yes can you squash it please!

@martinRenou martinRenou merged commit 2d6cbc9 into bqplot:master Aug 18, 2021
@SylvainCorlay
Copy link
Member

Note: I think this is a 0.13 thing since it is a significant change of behavior.

@martinRenou
Copy link
Member

Should we move forward introducing bqscales and removing threejs then?

I was also wondering if we should still maintain a 0.12.x branch?

@SylvainCorlay
Copy link
Member

Yes, we should start merging thing and create a 0.12.x branch pointing at the last commit before this PR.

@SylvainCorlay
Copy link
Member

@maartenbreddels just wanted to ping you here. You should be interested in this.

@maartenbreddels
Copy link
Member

Thanks for the ping.
Really love how this looks, a bit worried that the Figure class gets a bit bloated, and a reminder to us that we should think of composability a bit more for widgets (at least have a few strategies in mind for this).
In any case, really nice work @trungleduc !

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.

4 participants