fix(theme): prevent white flash during page load#81
fix(theme): prevent white flash during page load#81Mujojo03 wants to merge 1 commit intobitdevsnbo:masterfrom
Conversation
|
@Mujojo03 is attempting to deploy a commit to the BitDevsNBO Team on Vercel. A member of the Team first needs to authorize it. |
|
@okjodom |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| specs: | ||
| addressable (2.8.7) | ||
| public_suffix (>= 2.0.2, < 7.0) | ||
| base64 (0.2.0) |
There was a problem hiding this comment.
@Mujojo03 was this change intended to be part of this PR? Looks extraneous and accidental!
|
blocked by failing builds. @Mujojo03 please use rebase workflows, and avoid commits like 102de10 |
|
@okjodom |
|
Thanks @Mujojo03 for tackling this! The PR has the right intent and introduces some solid patterns (inline 🔴 Critical: Theme system disconnect —
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // If the toggleButton doesn't exist, stop gracefully | ||
| if (!toggleButton) return; | ||
|
|
||
| const savedTheme = localStorage.getItem("theme"); | ||
| if (savedTheme === "dark") { | ||
| body.classList.add("dark-mode"); |
There was a problem hiding this comment.
The early return prevents applying the saved theme on any page where #theme-toggle is absent, which can reintroduce the flash/mismatched theme. Apply the saved theme regardless, and only guard the parts that read/update toggleButton (click handler + textContent).
| const theme = storedTheme || (prefersDark ? 'dark' : 'light'); | ||
| document.documentElement.setAttribute('data-theme', theme); |
There was a problem hiding this comment.
This sets data-theme on <html>, but the updated theme CSS in this PR is driven by body.dark-mode and CSS variables (not [data-theme="dark"]). As a result, the “apply before styles load” step won’t actually affect styling unless you either (a) update CSS to key off html[data-theme="dark"] (or similar), or (b) set the same dark-mode class used by the runtime script.
| const theme = storedTheme || (prefersDark ? 'dark' : 'light'); | |
| document.documentElement.setAttribute('data-theme', theme); | |
| const theme = storedTheme || (prefersDark ? 'dark' : 'light'); | |
| // Keep data-theme for compatibility, but also apply the class used by theme CSS | |
| document.documentElement.setAttribute('data-theme', theme); | |
| if (theme === 'dark') { | |
| document.documentElement.classList.add('dark-mode'); | |
| if (document.body) { | |
| document.body.classList.add('dark-mode'); | |
| } | |
| } else { | |
| document.documentElement.classList.remove('dark-mode'); | |
| if (document.body) { | |
| document.body.classList.remove('dark-mode'); | |
| } | |
| } |
| * { | ||
| transition: none !important; | ||
| } | ||
|
|
||
| body.loaded * { | ||
| transition: all 0.1s ease; |
There was a problem hiding this comment.
Forcing transition: none !important on * and then transition: all on body.loaded * is very expensive (forces style recalculation widely) and also overrides intentional component-level transitions (and transition: all is particularly costly). Prefer scoping this to the properties involved in theme changes (e.g., background-color, color, border-color) and/or applying it only to elements that actually animate for theme switching (or using a body class like body:not(.loaded) to selectively disable).
| * { | |
| transition: none !important; | |
| } | |
| body.loaded * { | |
| transition: all 0.1s ease; | |
| body:not(.loaded), | |
| body:not(.loaded) * { | |
| transition: none !important; | |
| } | |
| body.loaded, | |
| body.loaded * { | |
| transition: background-color 0.1s ease, color 0.1s ease, border-color 0.1s ease; |
| <body> | ||
| <header>...</header> | ||
| <main>...</main> | ||
| <footer class="Footer"> |
There was a problem hiding this comment.
_includes/footer.html should not introduce <body>, <header>, or <main> tags; this will create invalid HTML (nested/duplicated document structure) when included inside the layout and can break styling and scripts. This include should render only the footer markup (starting at <footer class="Footer">).
| </footer> | ||
| </body> No newline at end of file |
There was a problem hiding this comment.
_includes/footer.html should not introduce <body>, <header>, or <main> tags; this will create invalid HTML (nested/duplicated document structure) when included inside the layout and can break styling and scripts. This include should render only the footer markup (starting at <footer class="Footer">).
| @@ -1,12 +1,19 @@ | |||
| // header.scss | |||
|
|
|||
| @use 'variables' as vars; | |||
There was a problem hiding this comment.
The variables import path is inconsistent with other files (@use './variables' as vars;). Depending on the Sass load path configuration, @use 'variables' may fail to resolve. Align this to the same relative path used elsewhere to avoid build failures.
| @use 'variables' as vars; | |
| @use './variables' as vars; |
| $base-font-size: 16px !default; | ||
|
|
||
| // Colors | ||
| $background-color: #000000; // black | ||
| $font-color: #ffffff; // white | ||
| $primary-color: #FFA500; // orange | ||
| $background-color: #000000 !default; // black | ||
| $font-color: #ffffff !default; // white | ||
| $primary-color: #F7931A !default; // orange |
There was a problem hiding this comment.
Variables are now split between this file and ./variables, and with Sass @use, variables aren’t implicitly global the way @import was. This makes it easy to end up with duplicated/conflicting configuration and “undefined variable” errors in modules. Consider moving all variable definitions into _variables.scss and either @forward them from a single entrypoint, or have each partial explicitly @use './variables' as vars; and reference vars.$... consistently.
| $small-screen: 400px !default; | ||
| $mid-screen: 768px !default; | ||
| $large-screen: 1024px !default; | ||
|
|
||
| @import './reset'; | ||
| @import './global'; | ||
| @import './header'; | ||
| @import './footer'; | ||
| @import './home'; | ||
| @import './post'; | ||
| @import './blog'; | ||
| @use './variables' as vars; | ||
|
|
||
| // @use './reset'; | ||
| // @use './global'; | ||
| // @use './header'; | ||
| // @use './footer'; | ||
| // @use './home'; | ||
| // @use './post'; | ||
| // @use './blog'; | ||
| @use './reset'; | ||
| @use './global'; | ||
| @use './header'; | ||
| @use './footer'; | ||
| @use './home'; | ||
| @use './post'; | ||
| @use './blog'; |
There was a problem hiding this comment.
Variables are now split between this file and ./variables, and with Sass @use, variables aren’t implicitly global the way @import was. This makes it easy to end up with duplicated/conflicting configuration and “undefined variable” errors in modules. Consider moving all variable definitions into _variables.scss and either @forward them from a single entrypoint, or have each partial explicitly @use './variables' as vars; and reference vars.$... consistently.
| <a href="/feed.xml">RSS Feed</a> | ||
| <div class="Footer-inner"> | ||
| <div class="Footer-source"> | ||
| <a href="https://github.com/BitDevsNBO/bitdevsnbo.org" target="_blank" rel="noopener nofollow">Source available on Github</a> |
There was a problem hiding this comment.
Correct the brand capitalization from "Github" to "GitHub" in the link text.
| <a href="https://github.com/BitDevsNBO/bitdevsnbo.org" target="_blank" rel="noopener nofollow">Source available on Github</a> | |
| <a href="https://github.com/BitDevsNBO/bitdevsnbo.org" target="_blank" rel="noopener nofollow">Source available on GitHub</a> |
No description provided.