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

[WIP] Fixed #31259 -- Added a dark theme in the admin module #12444

Closed
wants to merge 1 commit into from
Closed

[WIP] Fixed #31259 -- Added a dark theme in the admin module #12444

wants to merge 1 commit into from

Conversation

mimi89999
Copy link
Contributor

Some parts of the UI aren't themed yet, but most are. Please review code style and colors. If everything is OK, I will theme the entire admin UI.

@mimi89999
Copy link
Contributor Author

mimi89999 commented Feb 12, 2020

Screenshot_2020-02-12 Change Wydanie Django site admin
I made some screenshots to have a better overview.
Screenshot_2020-02-12 Select Podkategoria to change Django site admin
Screenshot_2020-02-12 Select user to change Django site admin
Screenshot_2020-02-12 Site administration Django site admin

@mimi89999
Copy link
Contributor Author

I changed a bit UI colors:
Screenshot_2020-02-12 Site administration Django site admin

@mimi89999
Copy link
Contributor Author

Please review. I think that most of the UI is themed now. I hope that there are no bugs left...

@mimi89999
Copy link
Contributor Author

Was using Sass ever condidered? It would greatly simplify the code as variables could be defined and colors could be automatically calculated. It could also provide CSS minification.

@knyghty
Copy link
Member

knyghty commented May 19, 2020

@mimi89999 Hi, I'm a big fan of this work 👍

First, to answer your question, I think there's a desire to make contributions to Django easy, and while everyone who knows Sass also knows CSS, the reverse is not necessarily true. But I think the bigger reason is it'd introduce a build process that has to happen somewhere, either when developing, which people invariably forget about (this happened with compressing JS, so it is no longer a requirement since nobody was following it). The other option is to do it during collectstatic but then we'd need every user to have a Sass compiler installed, which I think would be out of the question.

On the other hand though, the admin only supports modern browsers now, so you can use CSS variables and other modern CSS features. I realise they're a little clunkier but I have implemented dark themes with them fairly elegantly.

A couple of asks from my side:

  • The admin has seen some changes lately, particularly the new sidebar, but there have also been some other changes. It's probably worth rebasing and having a look through for anything that's now missing.
  • Can you run the admin through an accessibility checker and make sure the contrast is good everywhere? I realise that the current default theme doesn't have good enough contrast in many places, and while I hope to remedy this soon, it'd be good to set off on the right foot here.

@knyghty
Copy link
Member

knyghty commented Aug 9, 2020

@mimi89999, do you plan to continue this? I may take it up if not.

@mimi89999
Copy link
Contributor Author

Hello,
Sorry for not replying. I was busy and then forgot about your comment.

do you plan to continue this? I may take it up if not.

I think that it's almost finished. The core parts are themed. Some parts are still missing, but I will add them soon. There was no real feedback from project maintainers, so I didn't know if it's the way to go or I should change something.

First, to answer your question, I think there's a desire to make contributions to Django easy, and while everyone who knows Sass also knows CSS, the reverse is not necessarily true.

I think that learning Sass doesn't take long. To make basic changes almost no knowledge of Sass is needed.

But I think the bigger reason is it'd introduce a build process that has to happen somewhere, either when developing, which people invariably forget about (this happened with compressing JS, so it is no longer a requirement since nobody was following it). The other option is to do it during collectstatic but then we'd need every user to have a Sass compiler installed, which I think would be out of the question.

In a Django project I'm working on, we store compiled css in the repo and it's a nightmare because everybody forgets about updating it. Best would be to do it when submitting to pypi, but I don't know if it is possible.

The admin has seen some changes lately, particularly the new sidebar, but there have also been some other changes. It's probably worth rebasing and having a look through for anything that's now missing.

I will update it.

Can you run the admin through an accessibility checker and make sure the contrast is good everywhere? I realise that the current default theme doesn't have good enough contrast in many places, and while I hope to remedy this soon, it'd be good to set off on the right foot here.

I did run it through FF accessibility checker. I will check again after the update and will show you the results.

@knyghty
Copy link
Member

knyghty commented Aug 9, 2020

I didn't know if it's the way to go or I should change something.

It looks okay to me. Another way you could consider is to have a set of CSS variables, perhaps at the beginning of the main css file, or perhaps in its own file to make it easy to override. and then override them, e.g.

:root {
  --main-bg-color: #fff;
  ...
}

@media (prefers-color-scheme: dark) {
  :root {
    --main-bg-color: #000;
    ...
  }
}

This might end up being easier to maintain as you don't need to touch other files, but I think your approach is also fine.

@mimi89999
Copy link
Contributor Author

I rebased this PR, updated it and added theming of the new sidebar.

@knyghty
Copy link
Member

knyghty commented Aug 19, 2020

Hi, I've had a look through locally. It looks really good generally. I have found a few issues:

Firefox's accessibility check finds some contrast issues (with the button text, and all text in the filter box other than the title).
Screenshot 2020-08-19 at 22 30 13


This doesn't look beautiful. I know styling forms can be tricky, but can something be done here?
Screenshot 2020-08-19 at 22 31 11


I think there's a contrast issue with the label. I also think for a dark theme, this bright white background is quite jarring. Perhaps the buttons could be changed as well, but as they're SVGs it might not be worth it.
Screenshot 2020-08-19 at 22 32 02


These table headings need more contrast.
Screenshot 2020-08-19 at 22 33 10


Please also check admindocs. There are a few contrast issues and these code blocks should be made dark.
Screenshot 2020-08-19 at 22 33 49


Perhaps this could be a nicer colour, and same for the action "Go" button.
Screenshot 2020-08-19 at 22 35 29


I realise these buttons are disabled, but I think I should still be able to read them.
Screenshot 2020-08-19 at 22 47 36


I also think we should have the colours as CSS variables in one place as I think it'd be easier to maintain both themes side-by-side, but I'm not sure if it needs to be done here - @carltongibson do you perhaps have an opinion?

@mimi89999
Copy link
Contributor Author

Firefox's accessibility check finds some contrast issues (with the button text, and all text in the filter box other than the title).

I tested with webhint.io addon and it only reported issues with the header, which I changed. I will need to look deeper into this contrast issue.

This doesn't look beautiful. I know styling forms can be tricky, but can something be done here?

On my DE it looks good. Is it even possible to set the background of that dropdown from CSS?
Capture d’écran du 2020-08-20 12-20-13

I think there's a contrast issue with the label. I also think for a dark theme, this bright white background is quite jarring.

Widgets are not themed yet as reported previously. I will need to work on this.

I will fix other issues that those with widgets soon.

@knyghty
Copy link
Member

knyghty commented Aug 20, 2020

On my DE it looks good. Is it even possible to set the background of that dropdown from CSS?

It's browser-dependent. Firefox respects background-color and color. Chrome respects them but only when closed. For background-color it seems to choose a colour based on what you choose, and it chooses a color presumably to contrast with it. For example, with background: blue; color: black; on Chrome:

Screenshot 2020-08-20 at 12 45 31

Screenshot 2020-08-20 at 12 45 38

@ngnpope
Copy link
Member

ngnpope commented Aug 21, 2020

Chrome respects them but only when closed.

Looks as though that can be overcome by using -webkit-appearance: none: https://stackoverflow.com/q/9197992

@mimi89999
Copy link
Contributor Author

@knyghty

I realise these buttons are disabled, but I think I should still be able to read them.

Same issue in the light theme. When it's disabled, opacity is set to 0.3. I could change that only for the dark theme, but I think that it should be changed for both in a separate PR.

I themed the permission widget.

I also have a problem with checkbox buttons that seem to be impossible to theme in Firefox. Will need to check that in other browsers.

@matthiask
Copy link
Contributor

I have missed this PR before starting possibly related work in #13435. This pull request proposes using CSS variables for colors. Implementing a dark mode should be a matter of setting a few CSS variables (see here 95b489f#diff-9f1f1f2e0ee84be82ee1b3bd34f485a1L35-L69 )

@carltongibson
Copy link
Member

Hi @mimi89999 — I intend to merge @matthiask's #13435 this morning. If you can rebase off that and have this ready before the feature freeze on 14th Jan, I would be very happy to pull this into Django 3.2. 😀

Thanks for your work!

@carltongibson
Copy link
Member

Just unresolved the conversation about testing the fallback for Edge. (Surely that's supported now it's Chromium based... 🤔)

We need to think about adding a note to the new Admin Theming (and the release note) mentioning that Dark Mode is available (and installed, assuming we do/can use the media query.)

@mimi89999
Copy link
Contributor Author

Hello,
I started work on rebasing this PR, but encountered a blocking issue: #13435 (comment)

@mimi89999
Copy link
Contributor Author

Replaced by #13880.

@mimi89999 mimi89999 closed this Jan 12, 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
6 participants