-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add theming support #1760
Add theming support #1760
Conversation
for more information, see https://pre-commit.ci
…-debug-toolbar into add-theming-support
for more information, see https://pre-commit.ci
…-debug-toolbar into add-theming-support
@matthiask Any chance to get this reviewed, merged and released? The failing tests seems to be unrelated flakiness. |
Thank you for the PR. Given the context that this is a development tool, who is this particular change meant for and what problem(s) is it solving for them? |
There are more details in the related issue: #1757. In my case I'm developing a library that pre-configures Django Debug Toolbar in a certain way. The tests include screenshots with the toolbar open, therefore it would be important to ensure that the font family is consistent (to ensure that the screenshots are the same regardless of the developer's operating system). I acknowledge that this might be a relatively niche use case, but I think adding this feature should not hurt nor should it cause unnecessary maintenance burden. We're bringing the toolbar closer to how Django handles this as well (e.g. for the admin site). Additionally, this feature could be beneficial for plugin developers as well (in case they want to add screenshots to their tests). |
Thank you for the explanation! I think this PR needs to include the documentation on how to customize/utilize the theming support, along with explanations on what can be included in that theme (I assume that's a shorter list than what can't be). |
Alright, I'll extend the documentation, however, it will be pretty limited with only the font families at the moment. |
Documentation is extended now with a section on theming support. As the process is basically the same as for the Django admin site (see https://docs.djangoproject.com/en/4.2/ref/contrib/admin/#theming-support) I also documented it very similarly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
I found a typo; that should be fixed. The removal of the extrastyle
block is a stylistic choice; you don't have to follow that. I don't see why the additional block is necessary when you're using {{ block.super }}
anyway, but that's just me.
Now that I took a closer look I worry minimally that we use a generic sounding CSS property for the font family list. I'm quite sure it's safe enough to try but I wanted to raise the question if we shouldn't use --djdt
-prefixed properties. I think I'm slightly against it since for example --font-family-monospace
really should be a monospace font and we cannot hinder people from shooting themselves in the foot (or maybe rather applying lipstick to their foot or something)
Co-authored-by: Matthias Kestenholz <[email protected]>
Thank you for the comments! I thought about just using the I think prefixed variables would actually make sense, I haven't thought of that. That's because otherwise it will be way too easy to clash with either the website's own style sheets or with Django's variables. I'll change this to use a prefix instead in a moment. |
The spell check failed:
The psycopg3 failures are not related and can be ignored for this change, but you should fix the spell check test. Thanks! |
Thanks! |
Thank you @GergelyKalmar! Good work all around. |
Thank you all for the support! Glad to see this go through. |
Description
This PR adds theming support to the Django debug toolbar that works the same way as https://docs.djangoproject.com/en/4.2/ref/contrib/admin/#theming-support.
Fixes #1757.
Checklist:
docs/changes.rst
.