Skip to content
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

Fix scaling of RadioButtons #10780

Merged
merged 6 commits into from Apr 2, 2018

Conversation

Abdul-Miraj
Copy link
Contributor

@Abdul-Miraj Abdul-Miraj commented Mar 14, 2018

PR Summary

This fix scales the radio buttons correctly with the size of the legend by taking the height that is already calculated for each section of the legend, which is defined by the variable dy (line 984). I took the dy value and converted appropriately into a radius (by dividing the value by 2) and subtracting a very small unit to allow for some spacing for in between the buttons. I made it default back into the previously hard-coded value of 0.05 if the value of the radius is any bigger so in the case where there isn't many radio buttons, they wouldn't be huge. Previously the value of the RadioButtons were set to 0.05 thus when the space given for each radio button was less then the 0.05 unit the buttons would overlap onto eachother.

Note: The tests seem to be failing at the image comparison tests for the radio button. Not entirely sure why, the test pass locally on my own machine. Any insight would be helpful, thank you.

before

after

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant

@afvincent
Copy link
Contributor

Hum, about the failing image test comparison, I would suspect a Freetype issue as it is quite usual. See for example this section in the documentation.

On top of that you could try to add remove_text=True to the image comparison decorator, to at least partly mitigate the issue with most of the fonts (the texts in the widgets are likely to stay visible, but the labels, tick labels and titles shoud be hidden IIRC). Being there, you could also add style='mpl20' to be more future-proof.

@Abdul-Miraj
Copy link
Contributor Author

Abdul-Miraj commented Mar 15, 2018

I original thought I was using the correct Freetype, as I set my setup.cfg correctly but realized it wasn't actually using the version mlp wanted. I had to delete my build folder and recompile the project for it to use the correct version #6911. Thank you!

@br-Zhang
Copy link
Contributor

Based on the logs, it looks like CI is failing due to a PEP8 issue because of trailing whitespaces on line 93 of test_widgets.py.

@Abdul-Miraj
Copy link
Contributor Author

Abdul-Miraj commented Mar 15, 2018

Oh.. I completely missed that. I assume they were just pep8 warnings that were just making me aware that they were there. Thank you @br-Zhang

@dstansby dstansby added this to the v3.0 milestone Apr 2, 2018
Copy link
Member

@dstansby dstansby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Thanks a lot for this fix!

@dstansby dstansby merged commit d9b5b4e into matplotlib:master Apr 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants