-
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
Dark mode support #1913
Dark mode support #1913
Conversation
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.
Thanks for your contribution!
I think I like it. I wasn't convinced it's worth it at first, but the diff looks really clean and the changes are well localized. Thanks.
Co-authored-by: Matthias Kestenholz <[email protected]>
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! 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 https://github.com/django/django/blob/a09082a9bec18f8e3ee8c10d473013ec67ffe93b/django/contrib/admin/static/admin/js/theme.js#L14
box-shadow: | ||
inset 0 0 5px 2px #aaa, | ||
0 1px 0 0 #eee; | ||
box-shadow: inset 0 0 5px 2px #aaa, 0 1px 0 0 #eee; |
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.
This doesn't look as if it has been formatted by prettier v4 (using pre-commit
)?
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.
Yes! This can be used to avoid duplication. They still have it on their CSS, maybe in case JS is disabled by default... |
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? |
Ill push a change to this once i get home if it's needed |
Nope, I'm indifferent on that. |
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.
This looks really nice! My only gripe is that we're taking up an entire row of the toolbar for the toggle button. I'm not sure there's a better option though. I'm bringing it up to see if anyone else has an opinion.
For the record, I agree with this, but I think that this can be addressed later, in a different pull request. Thanks! |
Description
This PR aims to add native support for dark mode.
It adds a button to switch between themes.
Checklist:
docs/changes.rst
.