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

Improve inheritance diagrams #22273

Merged
merged 1 commit into from Dec 15, 2022
Merged

Conversation

QuLogic
Copy link
Member

@QuLogic QuLogic commented Jan 20, 2022

PR Summary

When Graphviz is given a page size (which Sphinx does by default), it will layout the whole graph, and then scale the entire thing down to fit. This means that text and lines on large diagrams are inconsistent with ones that fit.

Additionally, the default size is larger than the content width which means the browser will also scale the image down a bit. But the URL map is not scaled down, so the links will appear in the wrong spot.

To fix this, set the page size in Graphviz very large, and make the diagram scrollable with CSS.

Plus a few minor tweaks to font and line sizes now that images are a consistent scale.

PR Checklist

Tests and Styling

  • [n/a] Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • [n/a] New features are documented, with examples if plot related.
  • [n/a] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • [n/a] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).

@timhoffm
Copy link
Member

I'm not convinced of scrollbars. This significantly clips the contents and reduces usability. See the screenshots below (please ignore resolution or use the links - screenshots taken on a 4K display with 1.3x don't render well). Maybe the current diagrams don't look as nice, but they are more usable. One can be a better rendering using SVG, which we already had, but reverted in #17913 because of an sphinx issue with broken links in SVG. My preference:

get sphinx fixed and use svg > keep current state > use scrollbars

New:
grafik

Old:

grafik

@QuLogic
Copy link
Member Author

QuLogic commented Jan 21, 2022

SVG may fix the URL map, but I'm not so sure it will look better for consistency in the cases where everything gets scaled down (such as Artist above.)

@timhoffm
Copy link
Member

Not sure if it's my 4K screen which makes it look worse or better one way or the other. Appearance is mediocre either way. What counts for me is usability and there needing to scroll the diagram is a no-go. If the size is really not managable, it would even better to only have a link and show the diagram somewhere else rather than show a small viewport.

@tacaswell
Copy link
Member

Discussed on the call, we are going to for ugly but working, over broken.

When Graphviz is given a page size (which Sphinx does by default), it
will layout the whole graph, and then scale the _entire_ thing down to
fit. This means that text and lines on large diagrams are inconsistent
with ones that fit.

Additionally, the default size is larger than the content width which
means the browser will also scale the image down a bit. But the URL map
is _not_ scaled down, so the links will appear in the wrong spot.

To fix this, set the page size in Graphviz very large, and make the
diagram scrollable with CSS.

Plus a few minor tweaks to font and line sizes now that images are a
consistent scale.
Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

This works on the three diagrams I spot checked...

@jklymak jklymak merged commit 8e9f5ec into matplotlib:main Dec 15, 2022
@QuLogic QuLogic deleted the better-inheritance branch December 15, 2022 21:55
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

4 participants