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

feat: Let handlers add CSS to the pages, do so for Python handler #218

Merged
merged 4 commits into from
Feb 4, 2021

Conversation

oprypin
Copy link
Member

@oprypin oprypin commented Jan 16, 2021

Fixes #189

@pawamoy
Copy link
Member

pawamoy commented Jan 17, 2021

OK I read your code and it's good, there are just a few things that bother me:

  • do you think users will want to hardcode CSS directly in the extra_css attribute? Personally I don't think so, because either you write your handler+renderer in a Python package, with templates and therefore you can write CSS in the style.css, or you write your handler+renderer in a local Python module (per project), and therefore you can simply write your CSS file as an asset and add it to mkdocs.yml.
  • the current approach makes it mandatory for any theme to redeclare the full CSS even if it falls back to another theme which already handles most of the styling. Couldn't we include in the final site both CSS files, so that a theme only has to declare styling for the elements it overwrote?

Based on these two points: couldn't we "simply" copy over the CSS file of the chosen theme (if any) and the CSS file of its fallback theme (if any) to the final site, without reading them into an extra_css variable?

@oprypin
Copy link
Member Author

oprypin commented Jan 17, 2021

do you think users will want to hardcode CSS directly in the extra_css attribute?

Perhaps not, but maybe? Regardless, it's a necessary implementation detail (more at the bottom).
FWIW it's not "hardcode", it's very much possible to generate CSS dynamically in code, just that we're not using this here.

the current approach makes it mandatory for any theme to redeclare the full CSS even if it falls back to another theme

Is it a real problem to duplicate a little CSS? Conversely, your suggestion I think has a problem.
I don't think the "fallback theme" can generically qualify for being the source of CSS for every theme because, after all, it's also just a theme on its own. So one would be scared to add CSS to that theme just because it might affect other themes?
When overwriting HTML, you overwrite whole files, but here it doesn't work like that.
I also thought generally, how to eliminate this duplication. I didn't find any good way, and decided that it's not worth it.

couldn't we "simply" copy over the CSS file of the chosen theme (if any) and the CSS file of its fallback theme (if any) to the final site, without reading them into an extra_css variable

Now we get to the implementation details.

  1. I implemented this generically, so that there can be any number (0 to N) of active handlers. Which (or even how many) handlers got pulled in is not known until all Markdown is finished rendering.
  2. The declaration of which static files get included into the site is possible to change only before rendering all the pages (incl. Markdown).
  3. It is possible to change the content of files very late in the build process.
  4. Following from these, the best we can do is always specify exactly 1 file to declare. In the end it will contain a merger of all CSS specified by all handlers.
  5. The most convenient place to determine which CSS file / content a handler should read is inside its __init__ because all the path calculations are already there. Having done that, we just store it for later, because at that time / location there isn't anything that has access to write a file (or at least doing it inside __init__ would be weird).

@pawamoy
Copy link
Member

pawamoy commented Jan 31, 2021

Alright, not a fan of this string concatenation thing but it's a nitpick and I don't want to prevent a merge just for this (and I don't see any better way myself) 😄
Could you add a small paragraph in the docs? Maybe under handlers/overview/#code, telling that users can add CSS in the extra_css variable or in a style.css file? You don't have to be exhaustive, I'd just like to build the docs skeleton along with the features, so it can be improved later 🙂

@oprypin
Copy link
Member Author

oprypin commented Jan 31, 2021

Hmm I think documentation of per-theme templates is missing, and this CSS stuff is based on it, so I added both.

@pawamoy pawamoy merged commit 8dd4eb3 into mkdocstrings:master Feb 4, 2021
oprypin added a commit to mkdocstrings/crystal that referenced this pull request Feb 28, 2021
oprypin added a commit to mkdocstrings/crystal that referenced this pull request Feb 28, 2021
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.

Incorporate the recommended CSS style in the package
2 participants