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

Rustdoc dark theme improvements #56200

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@ddfreyne
Contributor

ddfreyne commented Nov 24, 2018

This PR combines a handful of changes — I’ll happily split them if asked.

The changes:

  • Adjust the lightness of the dark theme to mark the contrast in the light theme. This affects the sidebar color, but especially the borders, which are quite subtle in the light theme but not so in the dark theme.

  • Adjust the colors of the dark theme to match the colors of the light theme. Links are now blue in both themes, and the entity types have the same colors in both themes.

DEMO: oldnew ⬅️


For reference, this is what the light theme currently looks like:

light-a
light-b
light-c


Here is what the dark theme currently looks like:

dark-a
dark-b
dark-c


Here is what the dark theme looks like after the modifications:
dark-new-a
dark-new-b
dark-new-c

ddfreyne added some commits Nov 24, 2018

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Nov 24, 2018

Some changes occurred in HTML/CSS.

cc @GuillaumeGomez

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Nov 24, 2018

r? @steveklabnik

(rust_highfive has picked a reviewer for you, use r? to override)

@ddfreyne

This comment has been minimized.

Contributor

ddfreyne commented Nov 24, 2018

Some thoughts:

  • The theme switcher, search bar, and settings button are untouched, but it’d be nice to give them a dark theme, too.

  • prefers-color-scheme support would be neat, although support is very limited at the moment, and I think it’d require quite a bit more though to support it along with the currently-existing theme switcher.

  • .content .highlighted.mod and .content .highlighted.externcrate were duplicated in the old dark theme. I’ve removed the duplication, but there might be more.

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Nov 25, 2018

I'm not a big fan of the new changes actually. However, it's hard to have a review on pure color/UI changes... In last case, we can always provide this as a third theme. To be discussed.

cc @rust-lang/rustdoc

@steveklabnik

This comment has been minimized.

Member

steveklabnik commented Nov 25, 2018

I don't have super strong feelings.

@ddfreyne

This comment has been minimized.

Contributor

ddfreyne commented Nov 25, 2018

@GuillaumeGomez Can you elaborate on your concerns?

I understand that this PR has more than one change in it, and I can split it up in multiple PRs, if you like:

  • Make link/entity colors identical to the ones in the light theme
  • Adjust contrast to match that of the light theme
  • Adjust file structure to match that of the light theme
@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Nov 25, 2018

They're not supposed to be identical or anything (I'm talking about colors here). Also, it's very difficult to give an appreciation on such thing as colors: I just don't like the proposed changes, there is unfortunately not much to tell about it...

@ddfreyne

This comment has been minimized.

Contributor

ddfreyne commented Nov 26, 2018

Taste is personal and can’t be argued about, but these changes have merit beyond taste:

  • Matching link/entity colors leads to reduced cognitive load when switching between light and dark themes. Viewers won’t have to re-learn the meaning of the colors.

  • Matching contrast makes sense because the dark theme is not an explicitly high-contrast theme. A high-contrast theme could probably still be useful, but then it might also make sense to have a high-contrast light theme, too. (Compare to macOS Mojave, where the contrast stays the same when switching between its light and dark theme.)

  • Matching file structure makes sense for maintainability.

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Nov 26, 2018

Matching link/entity colors leads to reduced cognitive load when switching between light and dark themes. Viewers won’t have to re-learn the meaning of the colors.

I see your point but I don't feel the same about it. For me it's more like having a visual information than having a matching one. Also, when you switch to a theme you like, I doubt you oftenly go to another one so is it really importat?

Matching contrast makes sense because the dark theme is not an explicitly high-contrast theme. A high-contrast theme could probably still be useful, but then it might also make sense to have a high-contrast light theme, too. (Compare to macOS Mojave, where the contrast stays the same when switching between its light and dark theme.)

That'd be nice to have actually. If you're motivated to write such themes, please go ahead! (not sure if we can just merge them as defaults though...)

Matching file structure makes sense for maintainability.

I didn't understand this point, what are you talking about?

@ddfreyne

This comment has been minimized.

Contributor

ddfreyne commented Dec 1, 2018

Matching link/entity colors leads to reduced cognitive load when switching between light and dark themes. Viewers won’t have to re-learn the meaning of the colors.

I see your point but I don't feel the same about it. For me it's more like having a visual information than having a matching one. Also, when you switch to a theme you like, I doubt you oftenly go to another one so is it really importat?

I do switch between light and dark on occasion, depending on the time of day.

@GuillaumeGomez What is the advantage of having different link/entity colors?

Matching contrast makes sense because the dark theme is not an explicitly high-contrast theme. A high-contrast theme could probably still be useful, but then it might also make sense to have a high-contrast light theme, too. (Compare to macOS Mojave, where the contrast stays the same when switching between its light and dark theme.)

That'd be nice to have actually. If you're motivated to write such themes, please go ahead! (not sure if we can just merge them as defaults though...)

This PR is not about creating a high-contrast theme. Is it about making the contrast in the light and the dark theme the same.

@GuillaumeGomez What is the advantage of only the dark theme (not the light theme) having high contrast?

Matching file structure makes sense for maintainability.

I didn't understand this point, what are you talking about?

The file structure of the dark theme is not the same as the light one. See my first commit in this PR.

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Dec 1, 2018

The file structure of the dark theme is not the same as the light one. See my first commit in this PR.

It just adds a comment?

@QuietMisdreavus

Overall, i kinda like this PR. Keep in mind i don't ever use the dark theme (@GuillaumeGomez is the one that introduced it and maintains it), but i like the thought of making sure it is similar to the light theme, to make our appearance more consistent for people who are deciding between them.

Since there seems to be some extra discussion about the matching entity colors: One thing to note is that the colors in the light theme have been selected to make sure that there's not too many link kinds with similar colors. (There's overlap between functions and type aliases, but i don't think those are liable to be in a situation where they can be confused.) However, in the dark theme, structs and enums are both greenish, and so are functions. Look at the last screenshot of each theme, and you can see what i'm talking about. I think moving to the colors that the light theme is using at least lets us be consistent between them.

I will ask, though: Have you checked the color contrast between these colors and the background of the dark theme? I remember going over all the colors on the initial dark theme to make sure they reached a contrast ratio over 4.5:1.

@@ -27,10 +27,6 @@ h2, h3:not(.impl):not(.method):not(.type):not(.tymethod), h4:not(.method):not(.t
border-bottom-color: #DDDDDD;
}

.in-band {

This comment has been minimized.

@QuietMisdreavus

QuietMisdreavus Dec 3, 2018

Member

Was this class being used? Why was it removed from both themes?

This comment has been minimized.

@ddfreyne

ddfreyne Dec 8, 2018

Contributor

The headers had the in-band class set, but because this class sets the background color to the same background color of the page, it is not used.

@ddfreyne

This comment has been minimized.

Contributor

ddfreyne commented Dec 8, 2018

I will ask, though: Have you checked the color contrast between these colors and the background of the dark theme? I remember going over all the colors on the initial dark theme to make sure they reached a contrast ratio over 4.5:1.

@QuietMisdreavus I haven’t properly verified the contrast (will do that). I did adjust the brightness of the colors so that the contrast (as perceived by my eyes) matches the one in the light theme, though.

@ddfreyne

This comment has been minimized.

Contributor

ddfreyne commented Dec 8, 2018

It just adds a comment?

@GuillaumeGomez My bad — the other commits also make some changes to the file structure (removing redundant CSS statements). I will move the refactorings into their own commit.

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Dec 8, 2018

I like the changes you made to the files themselves, but not the color changes. Again: they're not supposed to be the same as the light theme. If you don't like it, I suggest adding a new theme directly.

@ddfreyne

This comment has been minimized.

Contributor

ddfreyne commented Dec 17, 2018

OK, I’m not sure I can do much because there seems to be fundamental disagreement on my changes — will close.

@ddfreyne ddfreyne closed this Dec 17, 2018

@ddfreyne ddfreyne deleted the ddfreyne:dark-theme-improvements branch Dec 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment