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

Dark Mode flash #1508

Open
jonchurch opened this issue May 2, 2024 · 12 comments
Open

Dark Mode flash #1508

jonchurch opened this issue May 2, 2024 · 12 comments
Assignees

Comments

@jonchurch
Copy link
Member

There's a flash on page load when user has dark mode set

We can inline the css and js to set up the page before it's rendered to the browser

Slowed down w/ connection throttling:
flash
related #1490

@jonchurch jonchurch added the bug label May 2, 2024
@crandmck
Copy link
Member

crandmck commented May 2, 2024

@chrisdel101

@chrisdel101
Copy link
Contributor

Thanks for the feedback. I think there might be an influx of bug reports on this one, which I thought would be the case.

I did notice this but didn't realize it would be such an issue. I can make this fix. Do you have any tips on the best way to fix it?

@jonchurch
Copy link
Member Author

No worries @chrisdel101, you made a good change, and issues will come in identifying gaps. Power of the crowd! If you want to hack on this please do, others may want to fix some of the rough edges too. If we didn't like the change we would just revert it.

@chrisdel101
Copy link
Contributor

Thank @jonchurch! I will hack.

chrisdel101 added a commit to chrisdel101/expressjs.com that referenced this issue May 30, 2024
- move dark-mode class to html tag
- rearrange head order
- add document ready checks
- remove script tags from markup
crandmck pushed a commit that referenced this issue Jun 10, 2024
* Issue #1508
- move dark-mode class to html tag
- rearrange head order
- add document ready checks
- remove script tags from markup

* PR #1524
- remove console.log
@chrisdel101
Copy link
Contributor

There appears to be some issue specifically with the GH pages build of this site. #1524 should have fixed it but did not. If there is someway to re-build the production branch maybe this would fix it?

I've setup a fork version of the express site also using GH pages with my local account and the flash does not appear to occur there: https://chrisdel101.github.io/my-expressjs.com/

@crandmck
Copy link
Member

Wow, that's weird. I honestly don't know how that could happen.

The only thing I can think of is turning off GHP and then turning it back on. That would bring the site down momentarily... Should I try that?

@chrisdel101
Copy link
Contributor

chrisdel101 commented Jun 27, 2024

Maybe it will fix on the next PR that gets pushed as this will reload it then?
I noticed #1526 is fixed but I don't see how it got fixed. Might be the same kind of thing (unless a fix was applied to #1526 that I just can't see). You can see the nav flash still happens on the link above.

@chrisdel101
Copy link
Contributor

chrisdel101 commented Aug 29, 2024

Just revisiting this since I hate this bug! It is still happening. We determined that it's something to do w/ the GH-pages build of this site. I'm unable to reproduce it when I deploy this same site to my own github (as seen above) but I'm not using any of the scripts, actions, etc, so it might be something there? I really have no idea...

I thought maybe we could set up a staging site, and I could get full access to that so I deploy to this same env, but it doesn't work that way :( https://docs.github.com/en/pages/configuring-a-custom-domain-for-your-github-pages-site/managing-a-custom-domain-for-your-github-pages-site#configuring-a-subdomain. I don't think I can get write access only to just a subdomain, and would need it for the entire repo (which I'm not asking for).

Is there anyway we could set up a full clone of this repo with all the pipelines? Then I could have full admin access to that. Don't now the work involved in that. It's just an idea. This flash is just so nasty and we'd need to reproduce it to fix it.

Any other ideas? Even if this isn't urgent, maybe we can work at it over time.

@bjohansebas
Copy link
Member

bjohansebas commented Aug 29, 2024

@chrisdel101, maybe instead of doing it in a separate JS file, we could do it as an inline script within the head, so that when the browser starts rendering the HTML file, it doesn't need to make a request to another file, avoiding any waiting time.

Another possibility is that within the script, the document is waiting for all the content to be loaded before executing the functionality. Therefore, you could try removing that event and attempt it directly.

@chrisdel101
Copy link
Contributor

Yeah maybe.

Attaching the class to instead of fixed the issue everywhere else iirc, i.e. locally, netlify preview, my personal GH. Just not in prod.

Removing the content loaded guards (if that's what you meant) causes errors, at least for me since it runs before the page and then fails. The errors can be solved w optional chaining document?.querySelector('.theme-toggle')?.addEventListener('click', toggleStorageTheme) but then listener is not added. So they are required if we want to have it inline in the head.

Since the script is called near the top of the file I doubt moving it inline will do anything, but we might as well try it!

If it fails we'd need to revert the commit though since having the JS inline is awful (IMO). We don't want to leave that unless it's required.

Have to see how busy @crandmck is these days since it'll take work on his end. I have a copy with this change done. I can make a PR with it, but will hold off pushing that until I hear back.

@crandmck
Copy link
Member

crandmck commented Sep 2, 2024

Go ahead and open the PR @chrisdel101 and let's see how it looks. I agree that this issue is quite annoying, even if it's just cosmetic.

LMK what you need from me.... I'm back from vacation, but as usual I can't promise any specific response time, since I work on this project in my spare time .

@bjohansebas
Copy link
Member

bjohansebas commented Sep 8, 2024

Looking at the requests made by the page, I noticed that when deployed to production, rocket-loader is injected (this is enabled in the Cloudflare configuration), which causes the dark-mode script to execute later.

image

Whereas on Netlify, which doesn’t inject any script that delays the JS, the script works correctly as it is loaded at the start of the page.

image
To fix the error, you just need to add this attribute data-cfasync="false" before the src, as shown in the documentation, and it will no longer be necessary to make the changes from this PR (#1593).

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

Successfully merging a pull request may close this issue.

4 participants