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

Add theming support #1760

Merged
merged 13 commits into from May 9, 2023
Merged

Add theming support #1760

merged 13 commits into from May 9, 2023

Conversation

GergelyKalmar
Copy link
Contributor

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:

  • I have added the relevant tests for this change. (Existing tests should be sufficient as this is just a simple refactoring at this point.)
  • I have added an item to the Pending section of docs/changes.rst.

@GergelyKalmar
Copy link
Contributor Author

@matthiask Any chance to get this reviewed, merged and released? The failing tests seems to be unrelated flakiness.

@tim-schilling
Copy link
Contributor

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?

@GergelyKalmar
Copy link
Contributor Author

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).

@tim-schilling
Copy link
Contributor

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).

@GergelyKalmar
Copy link
Contributor Author

Alright, I'll extend the documentation, however, it will be pretty limited with only the font families at the moment.

@GergelyKalmar
Copy link
Contributor Author

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.

Copy link
Member

@matthiask matthiask left a 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)

debug_toolbar/static/debug_toolbar/css/toolbar.css Outdated Show resolved Hide resolved
debug_toolbar/templates/debug_toolbar/base.html Outdated Show resolved Hide resolved
docs/configuration.rst Outdated Show resolved Hide resolved
Co-authored-by: Matthias Kestenholz <mk@feinheit.ch>
@GergelyKalmar
Copy link
Contributor Author

Thank you for the comments! I thought about just using the css block but then I also thought it would be nice to be consistent with Django. I don't mind either way, we can keep it simple and use the existing block.

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.

@matthiask
Copy link
Member

The spell check failed:

writing output... [ 44%] configuration
configuration.rst:343: : Spell check: Theming: Theming support.
configuration.rst:346: : Spell check: Roboto: you preferred Roboto instead of the default list of fonts you could add a.
Writing /home/runner/work/django-debug-toolbar/django-debug-toolbar/docs/_build/spelling/configuration.spelling
writing output... [ 55%] contributing

The psycopg3 failures are not related and can be ignored for this change, but you should fix the spell check test. Thanks!

@matthiask matthiask merged commit 29f795f into jazzband:main May 9, 2023
19 of 21 checks passed
@matthiask
Copy link
Member

Thanks!

@tim-schilling
Copy link
Contributor

Thank you @GergelyKalmar! Good work all around.

@GergelyKalmar
Copy link
Contributor Author

Thank you all for the support! Glad to see this go through.

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.

Make font families configurable
3 participants