Navigation Menu

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

Lower Text's FontProperties priority when updating #16694

Merged
merged 1 commit into from Mar 7, 2020

Conversation

omarchehab98
Copy link
Contributor

Co-authored-by: Robert Augustynowicz robert.augustynowicz@mail.utoronto.ca
Closes: #16389

PR Summary

When fontproperties is being used, it's default values are overwriting explicit font attributes such as size.

plt.xlabel("value", fontproperties='SimHei',size=20) # this will work
plt.ylabel("counts",size=20, fontproperties='SimHei')  # this doesn't

Before

fontproperties_presedence

After

fontproperties_presedence-expected_ svg

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

anntzer
anntzer previously requested changes Mar 6, 2020
Copy link
Contributor

@anntzer anntzer left a comment

Choose a reason for hiding this comment

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

This should not use an image comparison test (per https://matplotlib.org/devdocs/devel/testing.html#writing-an-image-comparison-test), but either use check_figures_equal or much more simply just check the resulting text object's properties.

Anyone can dismiss when handled.

@omarchehab98
Copy link
Contributor Author

@anntzer thank you for your feedback, I agree with the change you requested and have pushed a simpler unit test that asserts on the Text's size.

@anntzer anntzer removed their request for review March 6, 2020 11:02
@anntzer anntzer added this to the v3.3.0 milestone Mar 6, 2020
@omarchehab98
Copy link
Contributor Author

@timhoffm thank you for the good suggestion. I applied it. I think we're good to go, will it merge automatically?

Co-authored-by: Robert Augustynowicz <robert.augustynowicz@mail.utoronto.ca>
Closes: matplotlib#16389
@QuLogic QuLogic merged commit a712590 into matplotlib:master Mar 7, 2020
@QuLogic
Copy link
Member

QuLogic commented Mar 7, 2020

Thanks @omarchehab98 !

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.

“Size” ignored if placed before fontproperties
4 participants