Consider splitting custom and collapsible admonitions into their own package

jbms / sphinx-immaterial

Adaptation of the popular mkdocs-material material design theme to the sphinx documentation system

https://jbms.github.io/sphinx-immaterial/

Other

191 stars 29 forks source link

Consider splitting custom and collapsible admonitions into their own package #211

Closed choldgraf closed 1 year ago

choldgraf commented 1 year ago

I really like the collapsible and custom admonitions functionality of this theme. I was looking into adding something similar in the pydata and book themes, but then I took a look at the code here and realized that the sphinx logic it is written in a way that is fairly theme-agnostic. The CSS is obviously unique to this theme but the rules of how to turn the admonition into a details block etc could be reused.

I'm curious how folks feel about the idea of defining a new sphinx extension like "admonitions-extra" that contained the logic that this theme is currently using for collapsible admonitions. Then this could be supported in other themes via their own CSS. I'd certainly love to add support for this in the pydata theme!

Just wanted to throw the idea out there and see what y'all thought about it!

jbms commented 1 year ago

How about integrating it into sphinx itself?

choldgraf commented 1 year ago

I 100% agree that would be the best solution. If they are open to it then I think that'd be much better. Sometimes Sphinx can be a bit slow to act in these situations but I'd certainly support this move. If you open an issue or something let me know and I'm happy to add my support for the idea.

choldgraf commented 1 year ago

It's also not a ton of code, so in the short term it might be easiest for us to just copy over the module to the pydata repo, that way the API for users is the same, and if Sphinx supports it natively then we can all deprecate the module one day 🙂

I opened an issue in the pydata repo to discuss FYI. https://github.com/pydata/pydata-sphinx-theme/issues/1124

If we do reuse the code we will make sure to give attribution to you!

2bndy5 commented 1 year ago

This is all very flattering! Code is MIT licensed, so promising attribution seems generous.

Before we put the custom admonition feature together, there was already some interest in sphinx to offer title-less admonitions based on our previous (now removed) customized generic md-admonition directive: sphinx-doc/sphinx#10713 - you may recognize the user from the executablebooks org.

2bndy5 commented 1 year ago

FYI, the customized admonition icon is tightly coupled with the implementation for our inline icon role. Not sure if that is also something you want to copy/modify.

12rambau commented 1 year ago

coming from the pydata-sphinx-theme, I completely support the idea of creating a small sphinx extention for that. I'm more than happy to participate if you want to start a repository, I can start one myself next week. If the code is already there that's just a matter of sanityzing everything !

2bndy5 commented 1 year ago

just a matter of sanityzing everything

Yes, but some base case (or standard) of admonition CSS needs to be established. The RTD theme can possibly serve as the admonition style standard (mkdocs-material theme used to mimic that style until recently).

Here, we use a jinja template for the CSS which is based on mkdocs-material theme's CSS for custom admonitions. Some theme(s) may style the admonitions so differently that a "one style fits all" approach may not be ideal for all themes.

Alternatively, this proposed new extension could offer an option to override the CSS template to work with non-RTD-like admonitions. I'm imagining something similar to how the autosummary extension allows for custom templates.

jbms commented 1 year ago

I think the portions that would make sense to upstream into Sphinx would be:

the CustomAdmonitionDirective (presumably renamed to just SphinxAdmonition or something) and the associated modifications to visit_admonition and depart_admonition. That adds support for specifying a different title, and for collapsing. Since that involves monkey patching it would be particularly helpful to upstream it.
a config option for defining custom admonition types, based on a specified directive name, title, and list of classes. Unlike our sphinx_immaterial_custom_admonitions config option, it wouldn't make sense to specify such things as color or icon since those options are inherently theme-specific.

We would still retain our own sphinx_immaterial_custom_admonitions option since that allows specifying the icon and color.

If for some reason we can't upstream it, I think for this theme it would actually be preferable not to factor out the above portions into a separate extension, because I don't think it is worth the trouble of having an additional dependency just for about 100 lines of code. The greater complexity is in handling the CSS and icons, and that is theme specific.

2bndy5 commented 1 year ago

I think for this theme it would actually be preferable not to factor out the above portions into a separate extension, because I don't think it is worth the trouble ...

I was thinking the same. Our inline icon role role and consolidated CSS would make it difficult to factor out this feature.

12rambau commented 1 year ago

I updated the documentation from pydata-sphinx-theme side (https://github.com/pydata/pydata-sphinx-theme/pull/1155). By doing it I realized that icon management is really theme centric and establishing a split extention will probably be incompatible with all themes but alabaster.

I thus think this can be closed as a wontfix, what do other think ?

2bndy5 commented 1 year ago

We reached the same conclusion but didn't close this immediately in case there was more to be said.

choldgraf commented 1 year ago

For what it's worth, thanks for the discussion here folks, and thanks for working on this cool theme!