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

Inheritance Diagram Dark Mode Support #1834

Merged
merged 4 commits into from
May 29, 2024

Conversation

j9ac9k
Copy link
Contributor

@j9ac9k j9ac9k commented May 27, 2024

Apply CSS filtering to have the inheritance diagram conform to dark mode, and add a example in kitchensink for demo.

We also install graphviz on CI to test this.

The css will only apply to inheritance diagram and not to all graphviz output.

Addresses #820

--
Here are some screenshots of the rendered output on a section of the pyqtgraph docs. Prior to this PR, only the light image was shown.

Some relevant settings in the pyqtgraph's conf.py

# conf.py
...
graphviz_dot_args = ['-Gbgcolor=transparent']
graphviz_output_format = 'svg'  
inheritance_graph_attrs = dict(
    rankdir="LR",
    fontsize=14,
    ratio='compress',
    bgcolor='transparent',
)
...
Screenshot 2024-05-27 at 11 40 35 AM Screenshot 2024-05-27 at 11 46 32 AM

Copy link
Collaborator

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

This looks great - my only suggestion (doesn't need to block the PR If you don't have time) is that we add a graphviz demo to this PR so that we can see it in action in the docs and have a visual test for regressions

@timhoffm
Copy link
Contributor

The minimal gaphviz example from here should be sufficient to include.

@j9ac9k
Copy link
Contributor Author

j9ac9k commented May 27, 2024

This looks great - my only suggestion (doesn't need to block the PR If you don't have time) is that we add a graphviz demo to this PR so that we can see it in action in the docs and have a visual test for regressions

No problem doing an example.

The minimal gaphviz example from here should be sufficient to include.

Oh nice, but this PR won't impact that output, the CSS is limited strictly to the inheritance diagrams, not to any graphviz output from Sphinx.

@j9ac9k
Copy link
Contributor Author

j9ac9k commented May 27, 2024

I got an example working, fair warning that some changes needed to be made to docs/conf.py (primarily to set the graphviz output to SVG). I'm writing out text for the example right now...

@j9ac9k
Copy link
Contributor Author

j9ac9k commented May 27, 2024

nit-picks welcome on the example, not sure how much to write or not to write. (Will address styling issues from pre-commit later today when I'm on my computer next)

@j9ac9k j9ac9k force-pushed the dark-moode-inheritance-diagram branch from bb2d10a to 1b1fe1a Compare May 28, 2024 00:22
@j9ac9k
Copy link
Contributor Author

j9ac9k commented May 28, 2024

Hmm... the read the docs build doesn't look right!

https://pydata-sphinx-theme--1834.org.readthedocs.build/en/1834/examples/kitchen-sink/graphviz.html#graphviz

Any suggestions?

@choldgraf
Copy link
Collaborator

I saw this in the logs, maybe this is why?

WARNING: dot command 'dot' cannot be run (needed for graphviz output), check the graphviz_dot setting

Also you should link the new page in a toctree somewhere!

@j9ac9k
Copy link
Contributor Author

j9ac9k commented May 28, 2024

I saw this in the logs, maybe this is why?

WARNING: dot command 'dot' cannot be run (needed for graphviz output), check the graphviz_dot setting

Yeah I realized as soon as I posted 😆

Also you should link the new page in a toctree somewhere!

It should have been added to a toc in kitchen-sink/index.rst, the glob should capture it.

.. toctree::
    :titlesonly:
    :glob:

    *

Copy link
Collaborator

@drammock drammock left a comment

Choose a reason for hiding this comment

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

Thanks for adding this! doc nitpicks inline below :)

Additionally: need to add graphviz to the "doc" section of pyproject.toml. I think with the recent CI changes from @trallard and @Carreau it will be enough just do do that, and we won't need to edit any of the .github/workflow files to get the CIs to pass.

docs/examples/kitchen-sink/graphviz.rst Outdated Show resolved Hide resolved
docs/examples/kitchen-sink/graphviz.rst Outdated Show resolved Hide resolved
docs/examples/kitchen-sink/graphviz.rst Outdated Show resolved Hide resolved
docs/examples/kitchen-sink/graphviz.rst Outdated Show resolved Hide resolved
@j9ac9k j9ac9k force-pushed the dark-moode-inheritance-diagram branch from e475979 to 406ba22 Compare May 28, 2024 11:08
@j9ac9k
Copy link
Contributor Author

j9ac9k commented May 28, 2024

Not going to lie, feel a little weird tweaking your readthedocs.yaml and conf.py so much just for this one example 😅

EDIT: "that escalated quickly" - now installing graphviz as part of the CI process. This PR sure got invasive quickly!

@j9ac9k
Copy link
Contributor Author

j9ac9k commented May 28, 2024

Hmm...need to figure out how to get graphviz on each platform in CI... I'll give that a shot.

EDIT : I can use a 3rd party github action, or I can craft my own install steps. Crafting my own will be far more verbose. You all have any opinions on what would be preferred? Rolled my own thing, easy enough to do.

j9ac9k added 3 commits May 28, 2024 09:54
Apply CSS filtering to have the inheritance diagram conform to dark
mode.
To generate the example that has graphviz, the `dot` utility needs to be
available, which is not by default.  This commit makes the graphviz
application available in the CI services that build the example.
@j9ac9k j9ac9k force-pushed the dark-moode-inheritance-diagram branch from 7071b78 to 683c6d1 Compare May 28, 2024 14:00
Copy link
Collaborator

@drammock drammock left a comment

Choose a reason for hiding this comment

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

CIs are green! Thanks for that (and for adding intersphinx) @j9ac9k! Just 2 minor wording changes to the example page.

@trallard can you check if you're happy with the change to the CI config? (and feel free to merge if you are) Seems like the main alternative is https://github.com/marketplace/actions/setup-graphviz

docs/examples/graphviz.rst Outdated Show resolved Hide resolved
docs/examples/graphviz.rst Outdated Show resolved Hide resolved
@j9ac9k
Copy link
Contributor Author

j9ac9k commented May 28, 2024

CIs are green! Thanks for that (and for adding intersphinx) @j9ac9k! Just 2 minor wording changes to the example page.

@trallard can you check if you're happy with the change to the CI config? (and feel free to merge if you are) Seems like the main alternative is https://github.com/marketplace/actions/setup-graphviz

Those suggestions made it much better, thanks for the feedback!

I considered some of the 3rd party "actions" to install graphviz, but since we don't need a specific (or even recent) version of graphviz, and each OS has an package manager (also what's up with GHA on windows not have microsoft's winget package manger?!) it was trivial to install. If you all prefer to use another pre-made action, that's fine too.

@Carreau Carreau merged commit d7bbb79 into pydata:main May 29, 2024
30 checks passed
@j9ac9k j9ac9k deleted the dark-moode-inheritance-diagram branch May 29, 2024 13:04
ivanov pushed a commit to ivanov/pydata-sphinx-theme that referenced this pull request Jun 5, 2024
Apply CSS filtering to have the inheritance diagram conform to dark mode, and add a example demonstrating inheritance diagram in kitchensink.. 

We also install graphviz on CI to test this. 

The css will only apply to inheritance diagram and not to all graphviz output.

Addresses pydata#820
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants