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

Add a system for theming, use it to implement dark mode #323

Merged
merged 27 commits into from
Nov 12, 2020

Conversation

jlfwong
Copy link
Owner

@jlfwong jlfwong commented Nov 8, 2020

Dark mode:
image

Light mode:
image

Fixes #220

@jlfwong
Copy link
Owner Author

jlfwong commented Nov 8, 2020

@evanw whenever you have the chance to try this out, I'm definitely interested in feedback re: contrast & eye strain.

@coveralls
Copy link

coveralls commented Nov 8, 2020

Coverage Status

Coverage decreased (-0.7%) to 47.469% when pulling 7639bc4 on jlfwong/dark-mode into 5f32640 on master.

@evanw
Copy link
Contributor

evanw commented Nov 10, 2020

Thanks so much for working on this! I just tried it out and it looks fantastic. Switching to Speedscope late at night no longer feels like a punch to the eyeballs.

A few things:

  • In dark mode, the black bar at the top of the welcome screen no longer stands out, but is also slightly off of the background color. Not sure if that matters to you or not. If it does, I would either make the background black too (remove the bar) or make it dark grey instead of almost black (keep the bar).

  • Search results currently stand out much less in dark mode than in light mode. My intuition says that they should stand out about the same amount but obviously it’s just a color preference thing, so feel free to ignore. You can check out my branch for what I thought felt right.

  • It sounds like you’re already on to this but switching the system-level dark mode preference breaks the interface. I mention this because I actually do this (usually once or twice a day). Let me know if you want help debugging it.

@jlfwong
Copy link
Owner Author

jlfwong commented Nov 12, 2020

Okay, thanks for the feedback!

RE: title bar in dark mode, I agree it looks a liiitttle bit weird, but I don't think it's a big deal, and changing the color of that one specific thing would require me either introduce a new theme color just to handle that case or have theme-specific logic about hiding backgrounds, which I don't really want. So I'm going to leave that one alone.

RE: search result contrast, I just pushed a new commit to update that. Here's the before & after:

Before After
search results before search results after

Re: system switching, I figured out the bug, so that should be fixed too!

Also remove implicit ordering dependency between GLCanvas and other
views. If GLCanvas ended up being last, then it would always clear,
erasing everything we drew before we blit to the screen!
@jlfwong jlfwong merged commit a1384b0 into master Nov 12, 2020
@jlfwong jlfwong deleted the jlfwong/dark-mode branch November 12, 2020 09:51
@evanw
Copy link
Contributor

evanw commented Nov 12, 2020

Just gave this a try. The colors look great now!

FWIW I'm seeing Speedscope lag a lot with quite a few copies of this in my console:

Uncaught RangeError: Maximum call stack size exceeded
    at Context.setViewport (graphics.ts:422)
    at Context.resize (graphics.ts:493)
    at GLCanvas.maybeResize (application.tsx:97)
    at CanvasContext.onBeforeFrame (canvas-context.ts:63)
    at graphics.ts:497
    at Set.forEach (<anonymous>)
    at Context.resize (graphics.ts:497)
    at GLCanvas.maybeResize (application.tsx:97)
    at CanvasContext.onBeforeFrame (canvas-context.ts:63)
    at graphics.ts:497

The problem went away when I reverted 2a19c36, which is how I tested the colors. This was tested on master after you landed this PR. Not sure if this is due to me building it incorrectly or not. Just wanted to flag this in case it was real and you were about to do a release without catching this issue.

@evanw evanw mentioned this pull request Nov 12, 2020
@bjorn3
Copy link

bjorn3 commented Nov 12, 2020

I get the following on speedscope.app.

Uncaught Error: undefined
speedscope.074e1f5c.js:100:1
    _updateRenderTargetAndViewport https://www.speedscope.app/speedscope.074e1f5c.js:100
    clear https://www.speedscope.app/speedscope.074e1f5c.js:100
    onBeforeFrame https://www.speedscope.app/speedscope.074e1f5c.js:114
    resize https://www.speedscope.app/speedscope.074e1f5c.js:100
    forEach self-hosted:4243
    resize https://www.speedscope.app/speedscope.074e1f5c.js:100
    maybeResize https://www.speedscope.app/speedscope.074e1f5c.js:190
[...]
    resize https://www.speedscope.app/speedscope.074e1f5c.js:100
    maybeResize https://www.speedscope.app/speedscope.074e1f5c.js:190
    onBeforeFrame https://www.speedscope.app/speedscope.074e1f5c.js:114
    resize https://www.speedscope.app/speedscope.074e1f5c.js:100
    forEach self-hosted:4243
    resize https://www.speedscope.app/speedscope.074e1f5c.js:100
    maybeResize https://www.speedscope.app/speedscope.074e1f5c.js:190
    onBeforeFrame https://www.speedscope.app/speedscope.074e1f5c.js:114

@jlfwong
Copy link
Owner Author

jlfwong commented Nov 12, 2020

Rats. Looks like I broke this on retina displays somehow. Will try to fix.

@jlfwong
Copy link
Owner Author

jlfwong commented Nov 12, 2020

@evanw @bjorn3 Fixed in 1.12.1, and https://www.speedscope.app/ should be back to functional now on high DPI displays.

@bjorn3
Copy link

bjorn3 commented Nov 12, 2020

Thanks! It works now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dark theme
4 participants