-
-
Notifications
You must be signed in to change notification settings - Fork 7.4k
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
Horizontal radio buttons #13374
base: main
Are you sure you want to change the base?
Horizontal radio buttons #13374
Conversation
flake8 needs to pass 😉 https://travis-ci.org/matplotlib/matplotlib/jobs/489577517 |
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.
Thanks for the PR @mromanie !
Here are a couple of comments. I'll do a more thorough review once these are addressed.
Can you in addition add a what's new entry? See doc/users/next_whats_new/README.rst
lib/matplotlib/widgets.py
Outdated
""" | ||
if orientation not in ['vertical', 'horizontal']: | ||
raise ValueError("Invalid RadioButton orientation: %s" % orientation) |
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.
Can you add what are the possible values here?
Can you also add a mock test to make sure it runs, as well as a test to make sure this exception is raised properly.
I also think we need to update the examples to show case this (examples/widgets/radio_buttons.py)
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 have a shortcut function to check options like this and raise an appropriate error now, cbook._check_in_list
lib/matplotlib/widgets.py
Outdated
@@ -1007,7 +1007,12 @@ def __init__(self, ax, labels, active=0, activecolor='blue'): | |||
The index of the initially selected button. | |||
activecolor : color | |||
The color of the selected button. | |||
orientation : str | |||
The orientation of the buttons: 'vertical' (default), or 'horizontal'. |
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.
The keyword being a bit confusing here, I think it's worth detailing a bit more what orientation means (which is the set of radio buttons are on the same horizontal axis or vertical axis).
For the failing flake8 test, I would suggest adding the check directly to your text editor. |
Milestoning 3.2 as there has not been activity recently. @mromanie are you still interested to work on this? |
@timhoffm Apologies for my inactivity. I am indeed still very much interested in working on this. In fact, I think I managed to do it properly by using patches.Ellipse instead of patches.Circle and stretching them to look circular using the aspects of the axes and figure. In this way, the entire surface of the buttons is sensitive, unlike the current implementation with patches.Circle. I still need to clean up the auto-scaling of the size of the symbols, which at the moment still results in the symbols to overlap under certain circumstances, and I'll submit the whole lot for review. Shall I use the same PR for this? |
What about using a |
@ImportanceOfBeingErnest I'm not familiar with
Plus a couple more things to make it backwards compatible. Like I said earlier, setting circle_radius to a fixed value is not always satisfactory and I' fine-tuning it. |
...forgot to say, the condition to activate the Ellipse button is:
|
If you plot a |
OK, thanks! All things considered, I would stick to the solution with Ellipse because:
|
Not sure what the status is here. Since someone asked a question about horizontal buttons on stackoverflow, I thought it might be worth showing a solution over there: https://stackoverflow.com/questions/55095111/displaying-radio-buttons-horizontally-in-matplotlib/55102639#55102639 Of course this could also be used for this update. |
This looks mostly done, but needs a rebase and a small tweak to the checks. The squashing is something that affects vertical buttons too, so could be a separate PR. |
d6c2cbe
to
8ea3641
Compare
Selecting by closest button is now merged as #13268. Using a scatter plot to ensure circular buttons is merged as #24474. I pushed a rebase, dropping the old changes, fixed the review comments and adding a test/what's new entry. I also modified the implementation a bit so that it modifies the placement of the buttons instead of reversing the I'm also working on implementing the same change for |
8ea3641
to
035517e
Compare
An example of how this looks may be found in the what's new entry: https://output.circle-artifacts.com/output/job/26b48f05-0be7-417a-b57e-305a60aa66b9/artifacts/0/doc/build/html/users/next_whats_new/widget_horizontal.html |
A new optional keyword argument 'orientation' is introduced to control it. The new parameter defaults to 'vertical' for backwards compatibility.
035517e
to
f76df0a
Compare
PR Summary
Add the option to have horizontal RadioButtons. A new optional keyword argument 'direction' is introduced to control it. The new parameter defaults to 'vertical' for backwards compatibility.
PR Checklist