search

expressjs / expressjs.com

https://expressjs.com
Other
5.23k stars 1.42k forks source link

Dark Mode Flash Fix #1524

Closed chrisdel101 closed 3 weeks ago

chrisdel101 commented 1 month ago

This fixes issue #1508. Adjust setting of the dark mode class and moved the order of the head to load to run the JS first and add the dark-mode class, then load dark mode style sheet first, if required. The theme class is now on the html tag to run this all before DOM load.

If this is not the final solution, it is significantly better than the current situation, which is extremely jarring.

I'm noticing a flicker now when loading the page, in the nav bar. It's occurring on my local build with the latest changes, but is not occurring on the production site online. So I'm going to submit this PR so I can see the preview to check for the flicker.

EDIT: After checking, the flicker is there in preview, but is coming from the main branch for me. Doesn't seem to be caused by this PR.

netlify[bot] commented 1 month ago

Deploy Preview for expressjscom-preview ready!

Name Link
Latest commit 3f2d7d1431d2dfdbc89fb9f7b64f4fca2775967b
Latest deploy log https://app.netlify.com/sites/expressjscom-preview/deploys/6663580238aa7d00072a8692
Deploy Preview https://deploy-preview-1524--expressjscom-preview.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

crandmck commented 1 month ago

Also added @jonchurch since he opened the original issue.

chrisdel101 commented 1 month ago

I had made it a draft PR to check out that preview as I noted before, so I just made it a normal PR. ~Please do check for the flicker in the nav bar while reviewing this :)~ ~(It's possible the issue this is fixing is actually covering up the nav flicker means it's not caused directly by this PR)~

chrisdel101 commented 3 weeks ago

RE: previous comments about nav bar flicker. I think this is a separate issue #1526. So disregard that.

chrisdel101 commented 3 weeks ago

This has not fixed the problem :( Is this change pushed to production already?

If I look at the preview, the flash is not there, that's why I thought it was fixed. But the flash is there in prod.

chrisdel101 commented 3 weeks ago

I do see the code changes are in prod. So I guess I need to take another stab at this. I'm not sure why it's not working as expected.

I pulled the updated prod changes to my machine and the flash is NOT happening there. So I'm not sure how to debug this since it's only happening on the expressjs.com site, even thought I can see the changes in the markup there.

chrisdel101 commented 3 weeks ago

Maybe it's a GH pages issue? Whereas the preview is Netlify and not happening there? I can maybe try hosting my own GH pages instance of the site to solve this. I can't think of any other way I'd be able to find out what the issue is here.

chrisdel101 commented 2 weeks ago

I tried to push my own instance of the site to GH pages and it's not working. It's not inputting the expressjs.com part after my site name. This is a HUGE issue I run into every time I use pages, but I assumed that I'd be able to push this repo as is and it would work just using the settings in the repo.

How do you guys get around this when deploying to pages?

It's going to take alot of tinkering to get this to run on my own pages if I have to do it all manually

chrisdel101 commented 2 weeks ago

FYI: Hacked together a working version.