Add ability to toggle legend in the figure options dialog box#30128
Add ability to toggle legend in the figure options dialog box#30128ritvi-alagusankar wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
|
@rcomer gentle ping to help review this PR, this will be my first contribution to matplotlib. Please let me know if there are any changes to be made :) |
|
I’m sorry but I am not currently available for reviewing. I also am not familiar with this part of the library so it is unlikely I will review it when I am available again. |
|
@ritvi-alagusanka please add some tests for your code. You may wanna look at the tests for similar features as a model |
|
@story645 @melissawm I have added a test case for this feature. Could you please help review this PR |
|
Hi, I noticed Codecov reported missing coverage on this PR. Since I don’t have push access to the source branch, I opened a follow-up PR to add the missing tests: #30587. This is actually my first open-source contribution — happy to start by helping here! 🎉 Thanks again for the original work! |
|
Note that CodeCov failing is not necessarily a blocker to getting this PR merged. |
|
2> Note that CodeCov failing is not necessarily a blocker to getting this PR merged. https://matplotlib.org/devdocs/devel/development_workflow.html#automated-tests @rcomer Could you help review this PR :) |
timhoffm
left a comment
There was a problem hiding this comment.
Overall, I'm sceptical that the matplotlib core library should invest further into configuration GUI. It's a rabbit hole. Also, all needed API is public and stable, so that such GUIs can be developed to arbitrary complexity by third parties; see e.g. https://pylustrator.readthedocs.io/en/latest/.
| ) | ||
| for name, axis in axis_map.items() | ||
| ]), | ||
| ('Legend visible', axes.legend_ is not None), |
There was a problem hiding this comment.
AFAICS this does not work, because it does not cover the case of an existing legend with set_visible(False)
| # toggle legend visibility | ||
| if axes.legend_ is not None: | ||
| axes.legend_.set_visible(legend_visible) |
There was a problem hiding this comment.
I have an issue with this:
Let's say, we don't have a legend, then the checkbox "Legend visible" is unchecked.
If the user checks the box, surpisingly nothing will happen since, we don't have a legend.
Either we would need to generate a legend (not so great beacuse we'd need to assume the defaults, equivalent to a legend() call), or we'd have to deactivate the checkbox if no legend is available (not sure whether that is possible with the generic dialog concept).
PR summary
Added the ability to toggle legend in the figure options dialog box, solving issue #11109. Changes in the PR:
PR checklist