-
-
Notifications
You must be signed in to change notification settings - Fork 437
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
Add dark mode #931
Add dark mode #931
Conversation
@vsalvino Thanks for doing all this! I didn't know about the media queries for dark mode. I like the And yes, font choice will be controversial. I wish there were a way to say, "If the user set a default serif, then use it, otherwise Georgia." I think most people have not changed the default, and so get Times. :( |
Yea, the media query is pretty fun, and newer within the past year or so (MDN). Regarding the fonts, I thought the serif font looked a bit odd in dark mode, and it also seemed odd to see the names of source code files in a book-style font. Purely personal opinion. As it is your project you get the final say :) It is easy to change the fonts with the sass variables. Here are a few screenshots to compare: Using Georgia/serif (current style): Using the OS's system font stack, which is quite popular with websites nowadays (including github) - this is probably the most "natural" looking.
Using OS system font plus monospace for the index (looks most natural to me... probably due to seeing those modules as imports in the source code frequently): Of course, the least controversial design would be to use the browser defaults:
Feel free to check out my branch and play with the sass. Let me know what we think. On a related note, I would appreciate you trying it on a robust test suite that triggers every possible UI element to thoroughly test that it looks good in light and dark mode. |
Codecov Report
@@ Coverage Diff @@
## master #931 +/- ##
==========================================
- Coverage 94.09% 85.46% -8.63%
==========================================
Files 86 86
Lines 12089 12089
Branches 1210 1210
==========================================
- Hits 11375 10332 -1043
- Misses 580 1600 +1020
- Partials 134 157 +23
Continue to review full report at Codecov.
|
Also, it looks like the unit tests are failing due to the HTML change. Can you advise the appropriate way to fix those (i.e. does something need to be re-generated, hard-coded, etc.?). |
I'd love to see this merged. Indeed the tests seem to fail because of the
It seems to be the same error across the whole test log. For example, in I guess you should simply update these gold files to change |
find tests/gold/html -type f -name "*.html" -exec sed -Ei 's/<span class="(run .*)>(.*)<\/span>/<button type="button" class="\1 title="Toggle lines run">\2<\/button>/' {} \;
find tests/gold/html -type f -name "*.html" -exec sed -Ei 's/<span class="(mis .*)>(.*)<\/span>/<button type="button" class="\1 title="Toggle lines missing">\2<\/button>/' {} \;
find tests/gold/html -type f -name "*.html" -exec sed -Ei 's/<span class="(exc .*)>(.*)<\/span>/<button type="button" class="\1 title="Toggle lines excluded">\2<\/button>/' {} \;
find tests/gold/html -type f -name "*.html" -exec sed -Ei 's/<span class="(par .*)>(.*)<\/span>/<button type="button" class="\1 title="Toggle lines partially run">\2<\/button>/' {} \; Tests passing locally! |
Here is another ping for you 😬 |
Codecov Report
@@ Coverage Diff @@
## master #931 +/- ##
==========================================
+ Coverage 94.09% 94.10% +0.01%
==========================================
Files 86 86
Lines 12089 12129 +40
Branches 1210 1214 +4
==========================================
+ Hits 11375 11414 +39
Misses 580 580
- Partials 134 135 +1
Continue to review full report at Codecov.
|
I appreciate your persistence with this! I will merge it, and then make a few tweaks. I'm afraid I can't go along with the monospaced file names, but I'm going to resist the "change resistance" I feel on a few other points, and see what people think. Thanks! |
I went back to your original monospaced index page, and this is now released as part of coverage 5.2. |
Thanks for getting this released. Feel free to change the design as necessary! |
Fixes #858
<button>
instead of<span>
for actionable elements (<span>
is not navigable by pressing TAB on keyboard, the browser has no knowledge that it can be clicked).If the user's browser does not support the prefer dark theme media query, the light theme is visible as it is simply the default. This does not depend on any experimental features and will work fine in any browser.
The light theme looks mostly the same, there are only a few very minor changes. Colors were inspired by light and dark themes of VS Code editor.
Font settings were cleaned up a bit in the sass as well. This is somewhat opinionated, but Georgia/serif did not look very good in dark mode, therefore sans-serif is used. Also the index was changed to use monospace, so that the names of the files would nicely align in a tabular fashion. The browser's default sans-serif and monospace fonts are used, as most developers probably set the ones they like as the defaults in their browsers, rather than the report forcing a font on the user.
Let me know if you have any feedback - would love to get this merged in! I realize anything design related is always heavily opinionated, but hopefully these styles match up nicely and keep the same overall look and feel.
Lastly, thank you for this project. I have started using it heavily and really love the HTML report.