GeezerVison

I've just seen this right now and haven't checked out how it looks yet, but as a heads up, #36306 is using the correct color scheme, but missing the toggle. I've also opened https://github.com/nodejs/nodejs.dev/issues/1068 regarding what I would consider being something that should be occurring if we do indeed get a toggler.

@DerekNonGeneric Should I work on this PR, considering that there is another open PR addressing the same issue?

@AjayPoshak, I would continue addressing @aduh95's feedback.

I added a transition for light mode to dark mode here nodejs/nodejs.dev#1071
Demo link https://staging.nodejs.dev/1071/

@benhalverson, thank you for doing this. I would consider this a very high priority issue and don't really know who to reach out to about getting that merged.

/cc @nschonni

Also, Firefox reports that the color for links element doesn't have an accessible color ratio:

Colour and Contrast
4.34

Does not meet WCAG standards for accessible text.

This can be solved by adding a CSS rule:

.dark-mode a, .dark-mode a:active, .dark-mode a:visited {
  color: var(--green4);
}

/cc @nodejs/documentation

This is how it looks like in dark mode

@nodejs/website

I haven't checked the whole changes, just FYI CSS variables don't work on IE. I personally don't care about IE and I don't think anyone would want to use it, but there might be valid cases one is stuck with IE 🙂

Also, not sure about using const and personally I'm not a fan of foo && bar and the { block, but I'll leave all this for others.

Merry Christmas to all!

Yes, These changes don't work on IE

I haven't checked the whole changes, just FYI CSS variables don't work on IE. I personally don't care about IE and I don't think anyone would want to use it, but there might be valid cases one is stuck with IE 🙂

Also, not sure about using const and personally I'm not a fan of foo && bar and the { block, but I'll leave all this for others.

Merry Christmas to all!

These change would make appearance worst on IE, but I don't think it would make browsing the docs on IE impossible to do.

Also, not sure about using const and personally I'm not a fan of foo && bar and the { block

Those works on IE 11 FWIW.

CI: https://ci.nodejs.org/job/node-test-pull-request/35215/

After testing this PR with IE, I agree we should fix some issues before it landing. Also, it's quite annoying when navigating across pages that the JS doesn't remember my choice when I select a theme by clicking on the button. I've put a few suggestions to fix that.

There are also some issues because we are mixing colors set by CSS var and colors set with hex code. The issues is that IE understands only the latter, making text sometimes unreadable. I've tried to find the occurences that were causing visual issues, and put them in the diff below. If you want, I can push them directly to your branch if that's OK for you.

diff --git a/doc/api_assets/style.css b/doc/api_assets/style.css
index 4567cef53d..f2a038e420 100644
--- a/doc/api_assets/style.css
+++ b/doc/api_assets/style.css
@@ -6,6 +6,7 @@
   --black3: #0d111d;
   --white: #ffffff;
   --white-smoke: #f2f2f2;
+  --grey-smoke: #e9edf0;
   --red1: #d60027;
   --red2: #d50027;
   --red3: #ca5010;
@@ -21,6 +22,7 @@
   --gray5: #7a7a7a;
   --gray6: #333333;
   --gray7: #c1c1c1;
+  --grey8: #ddd;
 
   --color-brand-primary: var(--gray6);
   --color-brand-secondary: var(--green1);
@@ -31,6 +33,7 @@
   --highlight-background-color: var(--white-smoke);
   --color-links: var(--green1);
   --color-fill-side-nav: var(--gray6);
+  --api-stability-links-bg:  rgba(255, 255, 255, .4)
 }
 
 .dark-mode {
@@ -43,7 +46,7 @@
 
 .dark-mode code,
 .dark-mode tt {
-  color: #e9edf0;
+  color: var(--grey-smoke);
   background-color: var(--highlight-background-color);
 }
 
@@ -187,7 +190,7 @@ li.version-picker a span {
 }
 
 ol.version-picker {
-  background-color: #fff;
+  background-color: var(--white);
   border: 1px solid var(--color-brand-secondary);
   border-radius: 0 0 2px 2px;
   display: none;
@@ -224,14 +227,14 @@ ol.version-picker li:last-child a {
 }
 
 .api_stability {
-  color: #fff !important;
+  color: var(--white) !important;
   margin: 0 0 1rem;
   padding: 1rem;
   line-height: 1.5;
 }
 
 .api_stability * {
-  color: #fff !important;
+  color: var(--white) !important;
 }
 
 .api_stability a {
@@ -241,7 +244,7 @@ ol.version-picker li:last-child a {
 .api_stability a:hover,
 .api_stability a:active,
 .api_stability a:focus {
-  background-color: rgba(255, 255, 255, .4);
+  background-color: var(--api-stability-links-bg);
 }
 
 .api_stability a code {
@@ -257,7 +260,7 @@ ol.version-picker li:last-child a {
 }
 
 .api_stability_2 {
-  background-color: #5a8147;
+  background-color: var(--green2);
 }
 
 .api_metadata {
@@ -458,14 +461,14 @@ code.pre {
 }
 
 #intro a {
-  color: #ddd;
+  color: var(--grey8);
   font-weight: 700;
 }
 
 hr {
   background-color: transparent;
   border: medium none;
-  border-bottom: 1px solid #7a7a7a;
+  border-bottom: 1px solid var(--gray5);
   margin: 0 0 1rem;
 }
 
@@ -491,8 +494,8 @@ hr {
 }
 
 #toc .stability_0::after {
-  background-color: #d50027;
-  color: #fff;
+  background-color: var(--red2);
+  color: var(--white);
   content: "deprecated";
   margin-left: .25rem;
   padding: 1px 3px;
@@ -589,7 +592,7 @@ a code {
 #column2 ul li a.active:hover,
 #column2 ul li a.active:focus {
   font-weight: 700;
-  color: #fff;
+  color: var(--white);
   background-color: transparent;
 }
 
@@ -597,7 +600,7 @@ a code {
 #intro a:focus,
 #column2 ul li a:hover,
 #column2 ul li a:focus {
-  color: #fff;
+  color: var(--white);
   background-color: transparent;
 }
 

@aduh95 Thank you for valuable feedback. Please go ahead and push these changes to this branch.

CI: https://ci.nodejs.org/job/node-test-pull-request/35247/

Took author-ready off just to make sure this doesn't land prematurely.

CI: https://ci.nodejs.org/job/node-test-pull-request/35255/

Took author-ready off just to make sure this doesn't land prematurely.

I think this is ready to land, it would be nice to have it in the next release. @jasnell do you have objections, or do you want more people to review?

Just wanted to get confirmation that it was ready to go. The discussion was unclear. No objections!

CI: https://ci.nodejs.org/job/node-test-pull-request/35320/

CI: https://ci.nodejs.org/job/node-test-pull-request/35321/

CI: https://ci.nodejs.org/job/node-test-pull-request/35322/

CI: https://ci.nodejs.org/job/node-test-pull-request/35328/

Since this change is doc only, I think it's OK to ignore the unrelated CI failures. I went ahead and landed this. Thanks a lot @AjayPoshak, docs look much better now 🎉

Landed in 9ff555b