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

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

Closed
choldgraf opened this issue Jan 21, 2023 · 12 comments
Closed

Comments

@choldgraf
Copy link

choldgraf commented Jan 21, 2023

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
Copy link
Owner

jbms commented Jan 21, 2023

How about integrating it into sphinx itself?

@choldgraf
Copy link
Author

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
Copy link
Author

choldgraf commented Jan 21, 2023

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. pydata/pydata-sphinx-theme#1124

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

@2bndy5
Copy link
Collaborator

2bndy5 commented Jan 21, 2023

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
Copy link
Collaborator

2bndy5 commented Jan 21, 2023

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
Copy link
Contributor

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
Copy link
Collaborator

2bndy5 commented Jan 22, 2023

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
Copy link
Owner

jbms commented Jan 22, 2023

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
Copy link
Collaborator

2bndy5 commented Jan 22, 2023

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
Copy link
Contributor

I updated the documentation from pydata-sphinx-theme side (pydata/pydata-sphinx-theme#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
Copy link
Collaborator

2bndy5 commented Feb 19, 2023

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

@2bndy5 2bndy5 closed this as not planned Won't fix, can't repro, duplicate, stale Feb 19, 2023
@choldgraf
Copy link
Author

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

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

No branches or pull requests

4 participants