Add dark mode by AjayPoshak · Pull Request #36313 · nodejs/node

AjayPoshak · 2020-11-29T15:45:37Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

aduh95

Please remove unrelated spacing changes in template.html, it's quite hard to review.

DerekNonGeneric · 2020-12-01T04:51:04Z

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.

AjayPoshak · 2020-12-02T05:46:38Z

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

DerekNonGeneric · 2020-12-02T17:44:03Z

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

benhalverson · 2020-12-03T09:31:56Z

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

DerekNonGeneric · 2020-12-03T10:46:59Z

@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

aduh95

The gtoc could be improved on dark mode. The list item separator is black against the dark background:

diff --git a/doc/api_assets/style.css b/doc/api_assets/style.css
index 5dda1fd787..ca0faacb0d 100644
--- a/doc/api_assets/style.css+++ b/doc/api_assets/style.css@@ -152,7 +152,7 @@ em code {
#gtoc > ul > li {
display: inline;
- border-right: 1px #000 solid;+ border-right: 1px currentColor solid;
margin-right: .4rem;
padding-right: .4rem;
}

aduh95 · 2020-12-05T19:18:20Z

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-modea, .dark-modea:active, .dark-modea:visited {
color:var(--green4);
}

aduh95

Please remove the IIFE (unless there's a good reason for using them I'm missing?).

Otherwise, LGTM with or without my other suggestions addressed.

aduh95

LGTM except the link color, which should meet the WCAG standards for accessible text.

aduh95 · 2020-12-11T15:56:07Z

/cc @nodejs/documentation

AjayPoshak · 2020-12-18T14:59:32Z

This is how it looks like in dark mode

aduh95

LGTM, thanks a lot for your work, that's a pretty damn looking dark theme :)

I'm adding a few extra comments – but feel free to disagree, I think we can land it with or without it if no one has any objection.

Trott · 2020-12-24T16:55:56Z

@nodejs/website

Trott

LGTM. I believe there is a competing dark mode PR and I haven't looked to see if one implementation is preferable to the other.

XhmikosR · 2020-12-25T09:38:35Z

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!

aymen94 · 2020-12-25T16:36:58Z

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!

aduh95 · 2020-12-25T16:54:26Z

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.

nodejs-github-bot · 2021-01-03T17:01:47Z

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

aduh95

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.

Details

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;
}

AjayPoshak · 2021-01-04T05:05:53Z

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.

nodejs-github-bot · 2021-01-04T20:04:16Z

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

jasnell · 2021-01-04T21:16:11Z

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

nodejs-github-bot · 2021-01-04T22:27:21Z

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

aduh95 · 2021-01-05T19:16:30Z

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?

jasnell · 2021-01-05T19:20:49Z

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

nodejs-github-bot · 2021-01-06T08:17:54Z

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

nodejs-github-bot · 2021-01-06T09:39:03Z

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

nodejs-github-bot · 2021-01-06T10:16:32Z

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

nodejs-github-bot · 2021-01-06T11:00:21Z

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

aduh95 · 2021-01-07T13:27:46Z

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

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> PR-URL: #36313 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Nov 29, 2020

aduh95 requested changes Nov 29, 2020
View reviewed changes

Comment threaddoc/template.html Outdated
Comment threaddoc/template.html Outdated
Comment threaddoc/template.html Outdated
Comment threaddoc/template.html Outdated

AjayPoshak requested a review from aduh95 December 5, 2020 15:18

aduh95 requested changes Dec 5, 2020
View reviewed changes

Comment threaddoc/template.html Outdated
Comment threaddoc/template.html Outdated
Comment threaddoc/template.html Outdated

AjayPoshak requested a review from aduh95 December 6, 2020 06:49

aduh95 requested changes Dec 6, 2020
View reviewed changes

Comment threaddoc/template.html Outdated
Comment threaddoc/api_assets/style.css Outdated
Comment threaddoc/api_assets/style.css Outdated
Comment threaddoc/api_assets/style.css Outdated
Comment threaddoc/api_assets/style.css Outdated
Comment threaddoc/api_assets/style.css Outdated

AjayPoshak requested a review from aduh95 December 11, 2020 14:18

aduh95 requested changes Dec 11, 2020
View reviewed changes

Comment threaddoc/api_assets/style.css Outdated
Comment threaddoc/api_assets/style.css Outdated

AjayPoshak requested a review from aduh95 December 18, 2020 15:00

aduh95 approved these changes Dec 18, 2020
View reviewed changes

Comment threaddoc/api_assets/style.css Outdated
Comment threaddoc/template.html Outdated
Comment threaddoc/template.html Outdated
Comment threaddoc/template.html Outdated
Comment threaddoc/api_assets/style.css Outdated

Trott approved these changes Dec 24, 2020
View reviewed changes

willin approved these changes Jan 2, 2021
View reviewed changes

aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Jan 3, 2021

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 3, 2021

aduh95 requested changes Jan 3, 2021
View reviewed changes

Comment threaddoc/template.html Outdated
Comment threaddoc/template.html Outdated

aduh95 approved these changes Jan 4, 2021
View reviewed changes

aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 4, 2021

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 4, 2021

aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 4, 2021

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 4, 2021

jasnell removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 4, 2021

aduh95 closed this Jan 7, 2021

aduh95 force-pushed the feature/add-dark-mode branch from 038946e to 9ff555bCompare January 7, 2021 13:27

aduh95 merged commit 9ff555b into nodejs:masterJan 7, 2021

danielleadams pushed a commit that referenced this pull request Jan 12, 2021
doc: add dark mode
a956fb3
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> PR-URL: #36313 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

danielleadams mentioned this pull request Jan 12, 2021
v15.6.0 proposal #36889
Merged

targos pushed a commit that referenced this pull request May 1, 2021
doc: add dark mode
19e6120
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> PR-URL: #36313 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

danielleadams mentioned this pull request May 3, 2021
v14.17.0 proposal #38507
Merged

Uh oh!

Uh oh!

Conversation

AjayPoshak commented Nov 29, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

aduh95 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DerekNonGeneric commented Dec 1, 2020

Uh oh!

AjayPoshak commented Dec 2, 2020

Uh oh!

DerekNonGeneric commented Dec 2, 2020

Uh oh!

benhalverson commented Dec 3, 2020

Uh oh!

DerekNonGeneric commented Dec 3, 2020

Uh oh!

aduh95 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aduh95 commented Dec 5, 2020

Uh oh!

aduh95 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aduh95 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aduh95 commented Dec 11, 2020

Uh oh!

AjayPoshak commented Dec 18, 2020

Uh oh!

aduh95 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Trott commented Dec 24, 2020

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

XhmikosR commented Dec 25, 2020

Uh oh!

aymen94 commented Dec 25, 2020

Uh oh!

aduh95 commented Dec 25, 2020

Uh oh!

nodejs-github-bot commented Jan 3, 2021

Uh oh!

aduh95 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AjayPoshak commented Jan 4, 2021

Uh oh!

nodejs-github-bot commented Jan 4, 2021

Uh oh!

AjayPoshak commented Nov 29, 2020•
edited
Loading