-
-
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
Consistently handle multi-axis positioning and labels #5827
Conversation
Codecov Report
@@ Coverage Diff @@
## main #5827 +/- ##
=======================================
Coverage 88.23% 88.24%
=======================================
Files 309 309
Lines 63866 63915 +49
=======================================
+ Hits 56354 56399 +45
- Misses 7512 7516 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Thanks! I think this is more consistent and we can now delete the old axis swapping code that I think is now dead code because Would be good to get this fix into this release even though I don't think we can claim swapping axes is supported until labels are also fixed... |
@@ -839,7 +853,8 @@ def _update_plot(self, key, plot, element=None): | |||
Updates plot parameters on every frame | |||
""" | |||
plot.update(**self._plot_properties(key, element)) | |||
self._update_labels(key, plot, element) | |||
if not self.multi_y: | |||
self._update_labels(key, plot, element) |
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.
We should open an issue about this detailing what would be required...
With the labels fixed, I can go update the docs to say the Other than some of the known limitations (streaming data, fancier range streams, no dynamic labels) I think this nicely rounds out multi axis support for this release. |
ca24c25
to
a36141a
Compare
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.
Happy to see this merged once some of the windows tests go green.
Supersedes #5816