Repository navigation
Dark theme - #679
Dark theme#679acerv wants to merge 4 commits into
Conversation
Centralize color literals in CSS custom properties in order to allow theme overrides while keeping default styles identical. Signed-off-by: Andrea Cervesato <andrea.cervesato@mailbox.org> Assisted-by: Gemini 3.8 Flash
Add dark theme custom property overrides under html[data-theme="dark"] along with targeted Bootstrap and Selectize dark surface styling. Signed-off-by: Andrea Cervesato <andrea.cervesato@mailbox.org> Assisted-by: Gemini 3.8 Flash
Read the saved theme from localStorage on page load and set the data-theme attribute early to avoid a flash of unstyled content. Signed-off-by: Andrea Cervesato <andrea.cervesato@mailbox.org> Assisted-by: Gemini 3.8 Flash
Add a Light/Dark theme selector to the user profile settings box and persist the preference in localStorage. Signed-off-by: Andrea Cervesato <andrea.cervesato@mailbox.org> Assisted-by: Gemini 3.8 Flash
| user_profile = UserProfile.objects.get(user=self.user.id) | ||
| self.assertEqual(user_profile.items_per_page, old_ppp) | ||
|
|
||
| def test_user_profile_theme_setting(self): |
There was a problem hiding this comment.
I'm not sure if this test is useful. It does not seem to test any logic on the backend, just if the profile page has relevant elements. It does, as long as the elements are in the template... I don't see a lot of room for a regression here... Are any there any other tests like that in the codebase?
| <head> | ||
| <script> | ||
| (function() { | ||
| try { |
There was a problem hiding this comment.
Why is an exception caught and ignored here? If anything in this script throws, the theme won't get applied either way. Wouldn't it be better to get a stacktrace in the console then?
There was a problem hiding this comment.
I totally agree and indeed I was not sure about it. Thanks for the feedback, I will check how to make it properly.
| var savedTheme = 'light'; | ||
| try { | ||
| savedTheme = localStorage.getItem('pw_theme') || 'light'; | ||
| } catch (e) {} |
There was a problem hiding this comment.
Wouldn't it be better to make some sort of an error visible on the page instead of just ignoring the exception? Or at least not catch the exception at all, so something is visible in the console.
There was a problem hiding this comment.
Also, why localStorage and not a field in the profile model? I guess with the current approach, the user can set a different color scheme for each session. But also, has to set the theme manually for each session. Is this desirable?
And also, dark theme won't be supported without javascript (but I'm not sure if patchwork cares about that).
There was a problem hiding this comment.
This is a good point, I didn't check if patchwork was injecting JS code and I would like to wait for maintainers before proceeding, otherwise I might waste time on a PR that is not going to be merged anyway.
| try { | ||
| localStorage.setItem('pw_theme', val); | ||
| } catch (e) {} |
There was a problem hiding this comment.
Same thing with the catch here.
| float: right; | ||
| width: 49%; | ||
| } | ||
|
|
There was a problem hiding this comment.
(line irrelevant) I noticed that some buttons are white-on-white with the dark theme. Have you tested this with maintainer permissions? (you may have no maintainer permissions on any projects, even as a superuser, until you explicitly add yourself as a maintainer (admin -> users -> maintainer projects)).
Commit message and patch contents are gray-on-gray for me on both Chromium and Firefox.
And I don't know if the link and checks colors are not too bright/saturated for the dark background. But this may be controversial and I don't particularly care... This is more apparent fullscreen, less on the screenshot. For example, on https://git.kernel.org/ link colors have lower saturation when dark mode is enabled (in the browser/os, by prefers-color-scheme)
| --danger-color: rgb(217, 83, 79); | ||
|
|
||
| /* Base theme colors */ | ||
| --body-bg: #fff; |
There was a problem hiding this comment.
I manually checked if the default colors did not change by accident and I believe there are no mistakes here.
I think that CSS properties/variables is the right approach.
| var themeSelect = document.getElementById('theme-select'); | ||
| if (!themeSelect) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Is there actually any condition under which this branch may be taken? Unless the select gets deleted or renamed of course
| <td> | ||
| <select id="theme-select" name="theme"> | ||
| <option value="light">Light</option> | ||
| <option value="dark">Dark</option> |
There was a problem hiding this comment.
I think it would make sense to also support prefers-color-scheme. Many websites supports this as a third "Use OS color scheme" option.
There was a problem hiding this comment.
This may also be a simpler alternative for localstorage/profile field. Less moving parts.
There was a problem hiding this comment.
This is actually the right approach yes, we can even remove most of the JS code at that point
2 participants
I use patchwork on daily basis and the bright theme can be difficult to stand after many hours of reading. For this reason, I sent this patch-set adding a user based configuration to switch from light to dark theme if people need to. Since I'm not a web developer, these changes have been assisted by Gemini 3.8 Flash. All patches compile separately.