jazzband / django-debug-toolbar

A configurable set of panels that display various debug information about the current request/response.

https://django-debug-toolbar.readthedocs.io/

BSD 3-Clause "New" or "Revised" License

7.97k stars 1.03k forks source link

Dark mode support #1913

Closed TheRealVizard closed 1 month ago

TheRealVizard commented 1 month ago

Description

This PR aims to add native support for dark mode. It adds a button to switch between themes.

Checklist:

[x] I have added the relevant tests for this change.
[x] I have added an item to the Pending section of docs/changes.rst.

TheRealVizard commented 1 month ago

Thank you! One more thing. The Django admin panel only uses prefers-color-scheme in the JavaScript code to determine the initial value. This simplifies the CSS insofar as the variables for the dark mode do not have to be repeated. Do you think something like that would be a good idea here as well? See

Yes! This can be used to avoid duplication.

They still have it on their CSS, maybe in case JS is disabled by default... https://github.com/django/django/blob/ceaf1e2848583ba832cc74715da38c802b6b0671/django/contrib/admin/static/admin/css/dark_mode.css#L1C1-L38C26

matthiask commented 1 month ago

Ah yes, you're right. Maybe the difference is that the Django admin mostly works without JS while the debug toolbar is unusable without JS. They cannot fully depend on JS.

I would prefer removing the duplication and depending on the presence of JS some more, but it's not a requirement to merging this, at least not in my view. For me it's good.

@tim-schilling Maybe you have a different opinion?

TheRealVizard commented 1 month ago

Thank you! One more thing. The Django admin panel only uses prefers-color-scheme in the JavaScript code to determine the initial value. This simplifies the CSS insofar as the variables for the dark mode do not have to be repeated. Do you think something like that would be a good idea here as well? See

Yes! This can be used to avoid duplication.

They still have it on their CSS, maybe in case JS is disabled by default... https://github.com/django/django/blob/ceaf1e2848583ba832cc74715da38c802b6b0671/django/contrib/admin/static/admin/css/dark_mode.css#L1C1-L38C26

Ill push a change to this once i get home if it's needed

tim-schilling commented 1 month ago

I would prefer removing the duplication and depending on the presence of JS some more, but it's not a requirement to merging this, at least not in my view. For me it's good.

@tim-schilling Maybe you have a different opinion?

Nope, I'm indifferent on that.

matthiask commented 1 month ago

For the record, I agree with this, but I think that this can be addressed later, in a different pull request. Thanks!