Skip to content

Make server-side aware of dark mode setting #574

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

Closed
wants to merge 1 commit into from

Conversation

AaronC81
Copy link
Contributor

@AaronC81 AaronC81 commented Oct 7, 2020

This PR switches the dark mode setting to be stored in a cookie rather than in localStorage. The server-side can then bake-in the mode-dark class if the dark mode cookie is true, removing the flicker of light theme (shown in #573) before the JS runs!

The theme is still checked on page load in JS, in case the theme is changed but the browser has cached the page with the wrong theme or anything like that. It will also move the old localStorage dark mode setting into a cookie, if one is found.

Fixes #573.

This allows Rails to render the view with the correct theme.
By doing this, a flicker of the wrong theme should not occur.
@natematykiewicz
Copy link
Contributor

natematykiewicz commented Oct 7, 2020

Pages are cached in CloudFlare. The goal is that the Rails app doesn't receive many requests. I'm unsure what Colby will think of this.

@natematykiewicz
Copy link
Contributor

natematykiewicz commented Oct 7, 2020

In this PR, I fixed a similar issue with FontAwesome. #381

I think we could do the same thing -- move the JS out to its own pack that's not deferred and loads before the application JS.

Or if not its own pack, maybe we make a new single "now" type pack that the icons and the theme are in, so that there's only 2 files.

@AlexWayfer
Copy link
Contributor

This is what I've done in my website: AlexWayfer/alexwayfer.name@65832f4

Summary:

  1. CSS class of dark theme for <html>.
  2. CSS and JavaScript in <head>, before <body>.
  3. JavaScript is on DOMContentLoaded event.

@natematykiewicz
Copy link
Contributor

natematykiewicz commented Oct 8, 2020

The crux of the issue is how long it takes for the mode-dark class to be added to the body.

From what I see, Stimulus controllers are executed alphabetically. Renaming template-switch_controller.js to something earlier in the alphabet may help. When I worked on the FontAwesome problem, the biggest factor was the defer on the <script> tag. The JS took a long time to download and execute when deferred (which was intentional), so making the icons run immediately removed the flicker (by flicker, I mean, you'd come to the page and the icons would pop in a half a second later -- very similar to this problem).

The thing to keep in mind is that some of the JS is progressive enhancement (clicking the search box does something, for example), and some of it is vital to the layout of the page (choosing the correct theme, adding icons to the HTML).

You want the layout stuff to happen immediately, and the progressive enhancements (event listeners) to be deferred.

The CSS downloads and gets processed fast enough. I would personally recommend against inlining any JS / CSS. When JS and CSS are in their own files, their contents can be cached with a really long cache expiration. When it's in the HTML, you end up wasting bandwidth redownloading something that you could have just cached, because the HTML has a much shorter cache duration (because it's volatile due to layout changes of a static URL).

I'm not sure exactly how to solve this problem. If you move the theme switcher JS out of the application.js file, then you'll end up loading the Stimulus JS library twice unless you do code splitting.

When the theme switcher was made, I remember wishing there was a way to make it work with normal prefers-color-scheme: dark CSS again (like it used to). These CSS properties run much more quickly than deferred JS that adds a class.

Here's the JS in question:
https://github.com/rubyapi/rubyapi/blob/master/app/javascript/controllers/theme-switch_controller.js

Here's the corresponding PR:
#323

Here's my comment about wishing there was an "auto" option (though that would only work around this problem):
#290 (comment)

@natematykiewicz
Copy link
Contributor

Screen Shot 2020-10-07 at 11 39 55 PM

The application.js is 3.19MB (609KB Gzipped). I think processing all that JS is going to be slow as well. Why is that file so big? I don't remember it being so big.

@AaronC81
Copy link
Contributor Author

AaronC81 commented Oct 8, 2020

Thanks for your feedback! I didn't consider that there might be a server-side cache.

It looks like this approach isn't going to work out, so I'll close this and give it another shot soon, using the advice in your comments.

@AaronC81 AaronC81 closed this Oct 8, 2020
@colby-swandale
Copy link
Member

👋 The really large JS file size is my fault. the TL;DR is when I added the REPL page, I added the monoco-editor JS library which included a ton of stuff I'm not using.

I've addressed this in #577

But this doesn't solve the theme switcher causing the flickering if the JS file size get's big again, which I would like to resolve. It's really late here and I'll comment on this specifically in the next few days.

@natematykiewicz
Copy link
Contributor

Oh man, good call @colby-swandale

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.

When dark theme is enabled, light theme flickers visible when loading a page
4 participants