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

Dark theme CSS #351

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Dark theme CSS #351

wants to merge 6 commits into from

Conversation

nt1m
Copy link
Member

@nt1m nt1m commented Jul 19, 2018

No description provided.

@nt1m
Copy link
Member Author

nt1m commented Apr 22, 2019

@jankeromnes This should be ready for review now :) It now simply uses the system dark mode media query.

@nt1m nt1m requested a review from jankeromnes April 22, 2019 18:31
Copy link
Member

@Coder206 Coder206 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nt1m Thank you for the PR. I am having difficulty getting the dark theme to work in getting this to work on Firefox 66.0.3 and Firefox Nightly 68.0a1 (2019-05-02) despite having support for this the CSS query. The best I could achieve was:
Screenshot_2019-05-03 Janitor in Firefox Nightly.

To be clear, I am fine with using newer CSS queries provided that we have fallbacks for browsers with no support.

Otherwise, I am concerned that the title for this PR is misleading as there a significant amount of changes made to the light theme that seem to be unrelated to the dark theme (e.g. changing the background colour for a button hover)

@@ -38,6 +38,12 @@
left: 50%;
transform: translate(-50%, -50%);
}

@media (prefers-color-scheme: dark) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I admire the use of new CSS APIs, I was also wondering if there are plans to build a fall back for the many browsers that don't support this media query?

static/css/janitor-new.css Show resolved Hide resolved
static/css/janitor-new.css Show resolved Hide resolved
static/css/janitor-new.css Show resolved Hide resolved
static/css/janitor-new.css Show resolved Hide resolved
static/css/janitor-new.css Show resolved Hide resolved
static/css/janitor-new.css Show resolved Hide resolved
static/css/janitor-new.css Show resolved Hide resolved
static/css/janitor-new.css Show resolved Hide resolved
static/css/janitor-new.css Show resolved Hide resolved
@nt1m
Copy link
Member Author

nt1m commented May 4, 2019

@Coder206 Those changes are related, without them, the dark theme would have quite poor color contrast.

@nt1m nt1m requested a review from Coder206 May 4, 2019 14:26
Copy link
Member

@Coder206 Coder206 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was finally able to enable the Dark Theme by using Firefox Developer Edition and changing my OS's theme. Here are some changes I would like to see before approving:

  • The search bar for projects has a white background
  • Controls (forms and input boxes) are unusually dark, it doesn't feel like it fits with the dark theme
  • The insecure header background colour makes it hard to see the warning icon
  • All of the API examples have white backgrounds on white text

See:
Screenshot_2019-05-10 About - Janitor
Screenshot_2019-05-10 Log In - Janitor
Screenshot_2019-05-10 Projects - Janitor
Screenshot_2019-05-10 Projects - Janitor(1)
Screenshot_2019-05-10 API Reference - Janitor

Less critical but I would like to see more use of black (#000) instead of a dark gray.

Please note, my review was only able to cover Janitor in absence blog posts, containers and projects.

@jankeromnes jankeromnes removed their request for review October 13, 2021 13:16
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.

2 participants