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

doc: add dark mode styles #36306

Closed
wants to merge 2 commits into from

Conversation

jabyrd3
Copy link

@jabyrd3 jabyrd3 commented Nov 28, 2020

Pulled colors from nodejs.dev

Refs: #35793

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

Pulled colors from nodejs.dev

Refs: nodejs#35793
@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Nov 28, 2020
@@ -716,3 +716,19 @@ kbd {
overflow: hidden;
}
}
@media (prefers-color-scheme: dark) {
body {
background-color: #090c15;
Copy link
Member

Choose a reason for hiding this comment

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

Can you please remove the trailing whitespace?

Copy link
Author

Choose a reason for hiding this comment

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

sure thing, 2 minutes

color: #cbd4d9;
}
a:link {
color: #5fa04e;
Copy link
Member

Choose a reason for hiding this comment

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

Also the trailing whitespace here.

color: #5fa04e;
}
#column2.interior, #column2 ul {
background-color: #0d111d;
Copy link
Member

Choose a reason for hiding this comment

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

...and here as well.

@Trott
Copy link
Member

Trott commented Nov 30, 2020

Before:

image

After:

image

@Trott
Copy link
Member

Trott commented Nov 30, 2020

The colors on hovering appear low-contrast to me?

image

EDIT: Checked, and yeah, the contrast is only 1.42 which fails WCAG AA.

@Trott
Copy link
Member

Trott commented Nov 30, 2020

Are the different background colors between the nav pane on the left and the main display area intentional? They're similar...but not quite the same.

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! The color contrast issue is definitely something we'd want fixed before landing but all my other comments are things that can be ignored for now as minor.

@Trott
Copy link
Member

Trott commented Nov 30, 2020

@nodejs/website If we do this for the docs, I imagine we'd want to do it for the rest of the site as well? So I imagine you'll have some thoughts....

@silverwind
Copy link
Contributor

silverwind commented Nov 30, 2020

Needs to handle borders, dividers, etc. Essentially dark mode should remap every color. I suggest using CSS variables instead of hardcoding colors which among maintenance benefits also allows easier third-party theming.

Also, that shade of blue seems rather opinionated to me. suggest a neutral dark gray like #181818 for the background.

Also, I do think we need it toggleable via JS, not just via prefers-color-scheme because some users may like to switch manually.

@jabyrd3
Copy link
Author

jabyrd3 commented Nov 30, 2020

The colors on hovering appear low-contrast to me?

image

EDIT: Checked, and yeah, the contrast is only 1.42 which fails WCAG AA.

Good catch, changing link over color to white.

@jabyrd3
Copy link
Author

jabyrd3 commented Nov 30, 2020

Are the different background colors between the nav pane on the left and the main display area intentional? They're similar...but not quite the same.

They are; I mimicked https://nodejs.dev as requested in the original ticket. If we want to use different values though, I'm open to suggestions.

@jabyrd3
Copy link
Author

jabyrd3 commented Nov 30, 2020

Needs to handle borders, dividers, etc. Essentially dark mode should remap every color. I suggest using CSS variables instead of hardcoding colors which among maintenance benefits also allows easier third-party theming.

For the borders/dividers, the existing values for borders (usually lighter shades), meshed well with the values from https://nodejs.dev. As far as using CSS variables go, nothing else in either stylesheet for this set of pages is using CSS variables, so it seemed reasonable to me to match the existing patterns instead of refactoring the entire stylesheet, or only using variables for the darkmode theme.

Also, that shade of blue seems rather opinionated to me. suggest a neutral dark gray like #181818 for the background

I pulled all color values from https://nodejs.dev; I've got no preferences for these, if we want to change the values I'm open to suggestions.

Also, I do think we need it toggleable via JS, not just via prefers-color-scheme because some users may like to switch manually.

I'm amenable to this, I can push another commit to this PR this evening, EST, if its a blocking concern.

@jabyrd3 jabyrd3 requested a review from Trott November 30, 2020 14:05
@silverwind
Copy link
Contributor

silverwind commented Nov 30, 2020

One example of a border that will not work is the | between the menu entries on top. I'm sure there are more such cases. Any dark color in the stylesheet can cause this.

Regarding variables: I think it's reasonable to do now. This indirection for colors was not necessary before but is now with two themes.

Copy link
Contributor

@DerekNonGeneric DerekNonGeneric left a comment

Choose a reason for hiding this comment

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

This is a very good start! Something that I would be interested in seeing is having a way to toggle between the two modes. If that's not included in this PR, it can surely come later, though.

@jabyrd3
Copy link
Author

jabyrd3 commented Dec 1, 2020

This is a very good start! Something that I would be interested in seeing is having a way to toggle between the two modes. If that's not included in this PR, it can surely come later, though.

Yeah, I intended to add a toggle yesterday evening but my day job ended up running a bit long, will try to add something in today.

@aduh95
Copy link
Contributor

aduh95 commented Feb 5, 2021

@jabyrd3 do you still want to work on this? You'd need to rebase the PR to resolve the git conflict.

Also, it'd be nice to use the .dark-mode class that are now using the docs instead of the @media block.

@aduh95 aduh95 added the stalled Issues and PRs that are stalled. label Feb 19, 2021
@github-actions
Copy link
Contributor

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

@github-actions
Copy link
Contributor

Closing this because it has stalled. Feel free to reopen if this PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions.

@github-actions github-actions bot closed this Mar 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. stalled Issues and PRs that are stalled.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants