-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add dark/light theme swapper, make light theme less harsh, and some other small style changes #12565
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
Conversation
Paging @TheElectronWill and @pikinier20 as likely reviewers based on the conversation/assignment in the original issue |
} | ||
|
||
/* This needs to happen ASAP so we don't get a FOUC of bright colors before the dark theme is applied */ | ||
const initiallyDark = supportsLocalStorage && (localStorage.getItem("use-dark-theme") === "true"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If no theme has been explicitly selected by the user, the default should be dark if the system theme is dark, in CSS this can be queried using prefers-color-scheme: https://developer.mozilla.org/en-US/docs/Web/CSS/@media/prefers-color-scheme
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like window.matchMedia('(prefers-color-scheme: dark)')
should do the trick in JS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooooh that's pretty cool, I didn't know about that feature. I just pushed a commit to add that logic. I wired it up via JS as opposed to CSS since it needs to interact with the switch, and I think a CSS-only solution would require me to duplicate all of the variable settings (one for each media query).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is really nice!
Thanks a lot for the PR! The white footer in the dark theme is a bit troublesome to me. I find it weird because the contrast with the rest of the page is extreme, grabbing visual attention to the footer, which we do not want (or not this much). If all the icons were SVG we could swap their colors easily 🤔 Second point: have you checked the contrast of both versions? I'm worried that the background of the light theme might be too gray/dark (that is, not white enough). |
The CSS filter property should work on non-SVG images too: https://dev.to/pqina/poor-man-s-dark-mode-using-css-filter-211n |
I'll give the CSS invert+hueRotate filter a shot when I next have some time to play with this. Hopefully that'll work out to get the footer to fit the theme.
What I'm inferring from this question is that there's some kind of tool for measuring contrast on a page? I'm not familiar with one if that exists. All of this was just me fiddling until I decided I liked it, nothing so scientific. Admittedly I'm kinda biased on this one (I'm the redditor who made the post mentioned in #12525) - I felt there was too much contrast so I aimed to mellow it out a bit. I'll revisit the light theme during daylight hours and see if I can be happy with whiter background. |
Sorry, I should have explained that. Here are some standard guidelines for contrast on the web. Idea for another PR: you've mentioned on Reddit that it was difficult to navigate the links in the sidebar without wheel-scrolling. Maybe Mozilla's accessibility doc and the browser's accessibility checker can give useful hints to improve that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good job! I'm not eligible to decide about colors, though.
I've seen that social icons are not switched to black/white on theme switch but I think I can help you with that. There's already some logic with socials written in Scala.js and it would be nice to make it work together.
It would be much easier if we rewrote theme.js script to Scala.js but there I've got question for you (maybe my knowledge of JavaScript is not good enough): why did you need to add theme script without deffer attribute? AFAIK deffer
doesn't affect execution order. I ask because currently we attach Scala.js output with deffer
and I don't know if we should change it.
@pikinier20 Thanks! The reason for not using The alternative to avoiding Regarding the use of Scala.js, I personally disagree about it being easier. I found that making modifications to the Scala.js parts of scaladoc were the most painful, primarily due to the slow iteration time. It takes 5-10 minutes to compile after making changes (using @TheElectronWill Thanks for the clarification about the contrast stuff. I'll check those out later today. |
That seems like way too much, part of the problem is that scaladoc-js/compile calls |
Or maybe you're restarting sbt between every compilation round? |
Maybe it would be useful to create sbt task in scaladoc project that would update resources - compile JS and copy all scripts, styles etc. For me it takes like 10-20s to recompile scaladoc-js so it's not very inconvenient. |
Setting aside the compile time issue, I still think that setting up the theme script as a Scala.js project is a needless complication. As far as I can tell, the entire Compile / resourceGenerators += Def.task {
val jsDestinationFile = (Compile / resourceManaged).value / "dotty_res" / "scripts" / "searchbar.js"
sbt.IO.copyFile((`scaladoc-js` / Compile / fullOptJS).value.data, jsDestinationFile)
Seq(jsDestinationFile)
}.taskValue, Since the theme.js needs to not have the @TheElectronWill I checked the contrasts via Firefox's tool. This led to a few more minor adjustments (which I just pushed) but nothing major. The only complaints from the tool now are about the grayed-out keywords (e.g. |
@dylemma would you mind rebasing your PR? It looks like there's a conflcit now. |
@smarter sure thing. I'll take care of that after work today (I'm GMT-04:00 so that'll be a while). Looks like I need to do the same with my search UX PR too. |
Should be all set now (unless you would prefer a literal |
Rebases are preferred since they keep the history of the PR linear. |
c70e7ab
to
85574c7
Compare
Done |
Gah, something else must've got merged in the meantime so I have some more merge conflicts to figure out. |
Used the "poor man's dark mode" approach to deal with the images in the footer in dark mode, i.e. invert and hue rotate (without the hue rotate, the red scala3doc logo becomes blue) Also fix an issue where if you click "back to top" and then refresh the page, the "container" element gains the "expand" class, which causes the main signature (i.e. `class List[A] ...`) to become 'inline' instead of a block, and it also triggers the 6.5em left-margin that's intended for method signatures when they get expanded. Also consolidated the `footer` styles, since they were spread all over.
85574c7
to
be8fbba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this very nice contribution!
Aims to address #12525
The highlights:
>
icon match the body text color instead of the blue "active" color since that seemed to work better for both dark/light themes.margin
to separate the blocks, and only settingborder-left
, you don't get the little spikes.Before
After
Dark
Syntax Highlighting Before/After/Dark
I don't claim to be an expert in making choosing colors, but I think this is an improvement. My methodology was to start with the original sidebar color, which was
hsl(200deg 100% 14%)
aka#003047
, keep that hue, and adjust the saturation/lightness values to make sure the overall theme was relatively cohesive.One caveat about the dark theme is that the footer doesn't swap colors due to image-related difficulties. The "social links" icons are hard-coded to either black or white, and the Scala3doc logo is black, so making the footer dark would just make them harder to see. I briefly experimented with a CSS solution for the social icons, using
background-image: url(...)
, but then realized that the approach wouldn't work withCustom
icons (since the background-images needed to be hard-coded in the CSS). Eventually I decided not to have the footer swap from light to dark with the theme. As for the icons in the sidebar, the white icons seem to work just fine in either theme.For the theme swapper, I'm making it work by toggling a
class
attribute on the<html>
element, and setting up a:root.theme-dark
selector in scalastyle.css to override the color variables. I had to make some changes in the "backend" since it was hard-coded to setdefer="true"
on all scripts in the header, but in order to avoid a very jarring FOUC on page load, where you see the light theme before the script runs and swaps to dark theme, I had to make sure the "theme.js" runs ASAP, by omitting thedefer
attribute.