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

adding custom admonitions classes #183

Merged
merged 2 commits into from
May 27, 2020
Merged

Conversation

choldgraf
Copy link
Collaborator

@choldgraf choldgraf commented May 21, 2020

This replaces our Bootstrap admonition styles with some custom ones that are heavily influenced by mkdocs-materials. It removes all of the admonition translator code, so our admonitions will now behave just like any other Sphinx theme's admonitions (which I think is good).

Also adds some colors and icons for the major admonitions that I found in the RTD theme docs.

Here's how they look on my machine:

image

What do you think?

closes #182 closes #181

.. admonition:: If you add a name flag, it will be styled
:name: warning
.. admonition:: If you add a class flag, it will be styled
:class: warning
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is nice - we don't need to use our hacky extra code to achieve this effect now - this is functionality that "already works" in Sphinx

Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

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

This is looking really great !!

One thing I am wondering: maybe we could still use bootstrap's color variables, since they basically define those same 4 colors (success, danger, warning, info + primary as fall back when no class is given): https://getbootstrap.com/docs/4.5/getting-started/theming/#theme-colors
That way we don't hardcode those colors, and you can customize them by overriding those set of colors.

.. admonition:: If you add a name flag, it will be styled
:name: warning
.. admonition:: If you add a class flag, it will be styled
:class: warning

For example, this admonition block uses the following code:

.. code-block::

.. admonition:: If you add a name flag, it will be styled
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.. admonition:: If you add a name flag, it will be styled
.. admonition:: If you add a class flag, it will be styled

@@ -110,3 +110,160 @@ pre {
margin: 1.5em 0 1.5em 0;
box-shadow: 1px 1px 1px #d8d8d8;
}

// Admonitions CSS inspired by https://squidfunk.github.io/mkdocs-material/getting-started/
Copy link
Member

Choose a reason for hiding this comment

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

maybe directly put this in a separate file _admonitions.scss ?

@choldgraf
Copy link
Collaborator Author

choldgraf commented May 21, 2020

good points @jorisvandenbossche - I think I've addressed your comments!

The colors look more "bootstrap-y" now :-)

I also made "hint" be yellow, since I believe that's what other themes have used. But let me know if you'd really prefer green.

image

@choldgraf
Copy link
Collaborator Author

anything else to do on this one?

@jorisvandenbossche
Copy link
Member

I actually was thinking of the success/danger/warning/info colors, but I suppose those are not available with a -light variant? (but could maybe also use a function to lighten it: https://stackoverflow.com/questions/1625681/dynamically-change-color-to-lighter-or-darker-by-percentage-css-javascript)

Anyway, that can certainly be done as a follow up as well. So merge away if you are happy with it!

Some other possible follow-up thoughts:

  • It would be nice to use variables here in a way they are still overrideable by theme users (but that's probably covered by Make primary color (navigation hightlight, section titles) configurable #138. I also don't know if you can use css variables in sass and have them preserved in the css output)
  • We are adding some custom styling here (like the shadow), while some of those things are also covered by bootstrap classes. I am wondering if we can't make more use of that (like the sphinx-panels). Althought that then goes back somewhat to how it was before .. as that would need a custom translator to add those classes to the admonition node

cc @hoetmaaiers

@choldgraf
Copy link
Collaborator Author

Mmm - if you can give me a very specific look that you'd like for these admonitions colors, then I can try to achieve that via some transparency...but otherwise I'd prefer we tackle in follow-up PRs because I see that as being tricky doing back-and-forth over a PR.

For the custom classes stuff, I don't see it as too much of a hassle to just have our own CSS for some of this stuff. Or put another way, adding 5 lines of CSS seems way less-complex than creating a custom HTML5 translator to manually add bootstrap classes to things.

@choldgraf
Copy link
Collaborator Author

ok cool - I'm gonna merge this so we have time to get a feel for it, I also opened up #187 to track the suggestions you made @jorisvandenbossche 👍

@choldgraf choldgraf merged commit e6680a3 into pydata:master May 27, 2020
@choldgraf choldgraf deleted the admonitions branch May 27, 2020 16:34
@jorisvandenbossche
Copy link
Member

Thanks!

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.

ENH: improve the look of the admonition blocks Add classes to admonition blocks to distinguish between them
2 participants