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

luci-theme-bootstrap: add dark theme variant #5366

Merged
merged 1 commit into from Oct 8, 2021

Conversation

teohhanhui
Copy link
Contributor

Add a dark theme variant which is enabled by default when
prefers-color-scheme is set to dark.

Signed-off-by: Teoh Han Hui teohhanhui@gmail.com

@aparcar
Copy link
Member

aparcar commented Sep 15, 2021

Please show some screenshots.

@teohhanhui
Copy link
Contributor Author

Screenshots taken with the equivalent userstyle (on 21.02, but I've noticed the relevant style rules are practically the same while working on this PR):

https://github.com/teohhanhui/openwrt-luci-bootstrap-dark/tree/main/images/screenshots

@aparcar
Copy link
Member

aparcar commented Sep 15, 2021

Looks mostly fine to me, wondering if the extra code should be included by default but since people love working in the dark, this could be a good default feature.

@teohhanhui
Copy link
Contributor Author

teohhanhui commented Sep 15, 2021

I can try harder to make the changes even more minimal. There are always bound to be places where the colours would be wrong, most likely in other packages... But for things that are installed by default, and popular modules / apps, I believe users would be able to report any issues quickly if this is enabled by default.

@teohhanhui teohhanhui force-pushed the feat/bootstrap-dark branch 3 times, most recently from 73662a5 to ac33e2d Compare September 16, 2021 01:25
@jow-
Copy link
Contributor

jow- commented Sep 16, 2021

Great work. I suppose you could minimize the amount of scattered @media queries by factoring out the colors into CSS variables, then declare the variables with a different set of color values in a media query targeting light and dark preference each.

@teohhanhui
Copy link
Contributor Author

teohhanhui commented Sep 16, 2021

Do we still need to support IE? If not, yeah, we could use CSS variables.

https://caniuse.com/css-variables

Or even calc(), for that matter: https://caniuse.com/?search=calc

@jow-
Copy link
Contributor

jow- commented Sep 16, 2021

We already use calc() in various themes. I guess ditching MSIE support is fine.

@teohhanhui
Copy link
Contributor Author

teohhanhui commented Sep 16, 2021

Perfect. I'll work on using calc() for all the colour inversions / rotations(?)

Goodbye to hardcoded conversions.

@teohhanhui
Copy link
Contributor Author

I may have gone overboard with the var() and calc()...

Let me know if you'd like me to hardcode those values:

:root {
    --background-color-high: #fff;
    --background-color-medium: #f9f9f9;
    --background-color-low: #f5f5f5;
    --text-color-high: #404040;
    --text-color-medium: #808080;
    --text-color-low: #bfbfbf;
    --border-color-high: #ccc;
    --border-color-medium: #ddd;
    --border-color-low: #eee;
}

@media (prefers-color-scheme: dark) {
    :root {
        --background-color-high: #222;
        --background-color-medium: #282828;
        --background-color-low: #2c2c2c;
        --text-color-high: #fff;
        --text-color-medium: #bfbfbf;
        --text-color-low: #808080;
        --border-color-high: #555;
        --border-color-medium: #444;
        --border-color-low: #333;
    }
}

@jow-
Copy link
Contributor

jow- commented Sep 20, 2021

That looks quite fine already

@teohhanhui
Copy link
Contributor Author

That's the hardcoded version. The current version calculates everything based on adding / substracting lightness... 🙈

@jow-
Copy link
Contributor

jow- commented Sep 20, 2021

TBH, either is fine with me, both are a big step forward compared to the status quo. Personally I'd maybe favor the hardcoded version simply because it is easier to fiddle with while the calc based version is possibly better wrt. consistency in case someone touches the color palette.

@teohhanhui
Copy link
Contributor Author

Okay, so I think this is ready for review.

I'm keeping the calc() based version as that allows to maintain the relationship between the light / dark themes. And some of the variables also help in calculating other colours not in :root.

@jow-
Copy link
Contributor

jow- commented Sep 22, 2021

I gave it a quick try and on a first glance it looks quite good already. I think the button and input backgrounds need work though, imho they're far too bright in dark mode. That might be partially due to the fact that they don't have explicit colors or backgrounds to begin with.

For disabled inputs, selects etc. I would suggest the medium background color, low border color and the medium text color, for non disabled inputs I'd chose the low background, high border and high text color.

After some quick edits in Firefox inspector it looks like that for me (the Device type and Device name inputs are disabled):

image

As one can see, the rich dropdowns and the button elements require similar tweaks as well (they use gradients as backgrounds).

@jow-
Copy link
Contributor

jow- commented Sep 22, 2021

Here's the edits I did:

--- cascade.css.orig	2021-09-22 14:31:33.378955027 +0200
+++ cascade.css	2021-09-22 14:31:05.434836054 +0200
@@ -529,8 +529,10 @@
 	padding: 4px;
 	font-size: 13px;
 	line-height: 18px;
-	border: 1px solid #ccc;
+	border: 1px solid var(--border-color-high);
 	border-radius: 3px;
+	background: var(--background-color-low);
+	color: var(--text-color-high);
 }
 
 input,
@@ -688,10 +690,11 @@
 button[readonly],
 select[readonly],
 textarea[readonly] {
-	background-color: #f5f5f5;
-	border-color: #ddd;
+	background-color: var(--background-color-medium);
+	border-color: var(--border-color-low);
 	pointer-events: none;
 	cursor: default;
+	color: var(--text-color-medium);
 }
 
 select[readonly],
@@ -1376,14 +1379,14 @@
 .cbi-button {
 	cursor: pointer;
 	display: inline-block;
-	background: linear-gradient(#fff, #fff 25%, #e6e6e6) no-repeat;
+	background: linear-gradient(var(--background-color-low), var(--background-color-low) 25%, var(--background-color-high)) no-repeat;
 	padding: 5px 14px 6px;
 	text-shadow: 0 1px 1px rgba(255, 255, 255, 0.75);
-	color: #333;
+	color: var(--text-color-high);
 	font-size: 13px;
 	line-height: normal;
-	border: 1px solid #ccc;
-	border-bottom-color: #bbb;
+	border: 1px solid var(--border-color-medium);
+	border-bottom-color: var(--border-color-high);
 	border-radius: 4px;
 	box-shadow: inset 0 1px 0 rgba(255, 255, 255, 0.2), 0 1px 2px rgba(0, 0, 0, 0.05);
 	white-space: pre;
@@ -1489,10 +1492,10 @@
 }
 
 .cbi-dropdown:not(.btn):not(.cbi-button) {
-	background: linear-gradient(#fff 0%, #e9e8e6 100%);
-	border: 1px solid #ccc;
+	background: linear-gradient(var(--background-color-medium) 0%, var(--background-color-low) 100%);
+	border: 1px solid var(--border-color-high);
 	border-radius: 3px;
-	color: #404040;
+	color: var(--text-color-high);
 }
 
 .cbi-dynlist > .item:focus,
@@ -1568,14 +1571,14 @@
 .cbi-dropdown:not(.btn):not(.cbi-button) > ul > li {
 	min-height: 20px;
 	padding: .25em;
-	color: #404040;
+	color: var(--text-color-high);
 }
 
 .cbi-dropdown > ul > li .hide-open { display: block; display: initial; }
 .cbi-dropdown > ul > li .hide-close { display: none; }
 
 .cbi-dropdown > ul > li[display]:not([display="0"]) {
-	border-left: 1px solid #ccc;
+	border-left: 1px solid var(--border-color-high);
 }
 
 .cbi-dropdown[empty] > ul {

image

@teohhanhui
Copy link
Contributor Author

I'll get it done in the coming week. 🙏

@teohhanhui
Copy link
Contributor Author

Done.

Add a dark theme variant which is enabled by default when
prefers-color-scheme is set to dark.

Signed-off-by: Teoh Han Hui <teohhanhui@gmail.com>
@aparcar
Copy link
Member

aparcar commented Oct 6, 2021

Tested an looks good on my device, @jow- should I merge this?

@aparcar
Copy link
Member

aparcar commented Oct 8, 2021

I've tested and more and think it's ready to go, I'll review follow up PRs and issues.

@aparcar aparcar merged commit 6dd71ea into openwrt:master Oct 8, 2021
@teohhanhui teohhanhui deleted the feat/bootstrap-dark branch October 8, 2021 12:36
@hnyman
Copy link
Contributor

hnyman commented Oct 8, 2021

Has this PR/commit now turned the normal log text to hard-to-see light--gray ????

image

@aparcar
Copy link
Member

aparcar commented Oct 8, 2021

@hnyman thanks for the quick feedback! @teohhanhui could you please have a look?

@teohhanhui
Copy link
Contributor Author

turned the normal log text to hard-to-see light--gray

Definitely unintended. Sorry!

@teohhanhui
Copy link
Contributor Author

@hnyman Thanks for your patience. I've opened #5428. Let me know if that's good enough.

@hnyman
Copy link
Contributor

hnyman commented Oct 9, 2021

Looks marginally better, but apparently still not the earlier full black :-(
Is there a real reason for that?

I am not a CSS expert, but looking at your latest changes and trying to figure out the obfuscated mathematics (the hardcoded style would have been more clear...), I think that

  • originally there was no text color defined for this readonly log textarea (right?), so it was the default full black (0) against light gray (245), a contrast of 245.
  • Now it seems to be 64 (25% grey) against light gray (249?) , contrast of only 175, right?

I think that there should still a "full black" colortype (0) used here, (which you might then invert to 192, if 255 would be too bright against dark).

image

@jow-
Copy link
Contributor

jow- commented Oct 9, 2021

@hnyman - please give #5423 a try, it solves the log output issue among a couple of others.

@hnyman
Copy link
Contributor

hnyman commented Oct 9, 2021

Thanks jow, with #5423 black is black again ;-)

teohhanhui added a commit to teohhanhui/openwrt-luci-bootstrap-dark that referenced this pull request Oct 22, 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
Development

Successfully merging this pull request may close these issues.

None yet

4 participants