Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.1k
Fix Legend.set_draggable() with update="bbox"#25428
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Changes fromall commits
File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -77,7 +77,7 @@ def finalize_offset(self): | ||
| if self._update == "loc": | ||
| self._update_loc(self.get_loc_in_canvas()) | ||
| elif self._update == "bbox": | ||
| self._update_bbox_to_anchor(self.get_loc_in_canvas()) | ||
Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Does that mean none of this is tested? ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. It only gets triggered when dragging a legend, afaik mpl isn't testing any interactivity, is it? Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. I think it's possible to test interactivity. However, whoever put the typo in didn't test, or tested the wrong thing, so perhaps add to the PR description whatever test you are using to ensure this is the correct fix? Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. It is possible to test the interactive code, but I think that is way too big of an ask to get a 1 line patch in! Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Yes, sorry, I wasn't meaning to ask for that if its hard, but at least some idea how this should be tested manually. Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. @daniilS, since you seem to be doing some interactive PRs, you might be interested in taking a look here for how zoom and other interactive things like widgets are tested:https://github.com/matplotlib/matplotlib/blob/b636e9ed82b930ad4bde0b51360855fa79a7b438/lib/matplotlib/tests/test_backend_bases.py#LL142C3-L142C3 ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. @greglucas good point - I think all my other PRs were visual changes or toolbar behaviour which is hard to test automatically, but I might be able to write a test for dragging legends as the interaction is happening entirely within the axes. | ||
| def _update_loc(self, loc_in_canvas): | ||
| bbox = self.legend.get_bbox_to_anchor() | ||