feat: implement theme toggle functionality and enhance dark mode styles #140 - #139
feat: implement theme toggle functionality and enhance dark mode styles #140#139theavitw wants to merge 2 commits into
Conversation
theavitw
changed the title
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/app/globals.css">
<violation number="1" location="frontend/app/globals.css:89">
P1: The dark-mode color remapping uses substring selectors, so semantic solid colors such as `bg-red-500` and `bg-green-500` also match the `bg-red-50`/`bg-green-50` selectors. Their backgrounds are then replaced with the same translucent blue value via `!important`, making red/green health indicators blue in dark mode. Matching complete class tokens or using explicit dark variants would preserve those status colors.</violation>
<violation number="2" location="frontend/app/globals.css:93">
P2: Dark mode collapses success, error, and warning surfaces into the same blue tint, removing the color distinction used for Good/Poor ratings, validation errors, and selected survey responses. Preserve distinct semantic dark tints for green, red, and amber classes.</violation>
<violation number="3" location="frontend/app/globals.css:143">
P1: In dark mode, every button is forced to `background-color: var(--surface) !important` and `color: var(--foreground) !important`, so existing primary actions lose their Tailwind colors and white text. For example, the login submit button in `frontend/app/login/page.tsx` uses `bg-indigo-600 text-white`, but this global rule overrides both declarations; the same applies to the dashboard and admin action buttons. Restrict this generic control rule to form controls, or provide button-specific dark styles instead.</violation>
</file>
<file name="frontend/app/login/page.tsx">
<violation number="1" location="frontend/app/login/page.tsx:106">
P3: The per-element dark background utilities added here (dark:bg-slate-900/800, dark:bg-amber-950/40, dark:bg-red-950/*, dark:bg-indigo-950/70, and the dark:from/to-slate gradient on the demo panel) are overridden by the broad `!important` rules in app/globals.css (e.g. `html.dark [class*='bg-white']`, `[class*='bg-amber-50']`, `[class*='bg-red-50']`, `[class*='bg-indigo-100']`, `[class*='bg-gradient-to-br']`) that match the base light class still on the same element. They therefore never take effect; dark backgrounds come from the global overrides instead. Either drop these redundant dark:bg classes or make the dark mode styling consistent (e.g. remove the conflicting global overrides) so the intent isn't misleading.</violation>
</file>
<file name="frontend/components/ThemeToggle.tsx">
<violation number="1" location="frontend/components/ThemeToggle.tsx:22">
P2: When browser storage is blocked or unavailable, the global theme bootstrap safely falls back, but `ThemeToggle` still performs uncaught `localStorage.getItem` and `setItem` calls. In privacy-restricted or storage-disabled contexts this throws from the client effects on every page containing the root layout, leaving the toggle uninitialized and producing an uncaught runtime error. The component should treat storage as optional, just as the bootstrap script does.</violation>
<violation number="2" location="frontend/components/ThemeToggle.tsx:27">
P2: A saved dark preference is briefly treated as light during mount. The layout bootstrap script has already applied the saved theme before hydration, but this component starts with `theme` set to `'light'`; the `[theme]` effect therefore applies and persists light before the initialization effect's `setTheme(initialTheme)` update is reflected, then applies dark again. This can cause a theme flash and an unnecessary overwrite of the user's saved preference during startup.</violation>
<violation number="3" location="frontend/components/ThemeToggle.tsx:39">
P2: After the first page load, toggling the theme updates `data-theme` and the `dark` class but leaves the inline `document.documentElement.style.colorScheme` set by the layout bootstrap script unchanged. As a result, native controls and other browser UI can remain in the old color scheme after a toggle, even though the page CSS has switched themes. Updating the inline property whenever `theme` changes keeps the browser color scheme synchronized.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| } | ||
|
|
||
| export default function ThemeToggle() { | ||
| const [theme, setTheme] = useState<Theme>('light'); |
There was a problem hiding this comment.
P2: A saved dark preference is briefly treated as light during mount. The layout bootstrap script has already applied the saved theme before hydration, but this component starts with theme set to 'light'; the [theme] effect therefore applies and persists light before the initialization effect's setTheme(initialTheme) update is reflected, then applies dark again. This can cause a theme flash and an unnecessary overwrite of the user's saved preference during startup.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/components/ThemeToggle.tsx, line 27:
<comment>A saved dark preference is briefly treated as light during mount. The layout bootstrap script has already applied the saved theme before hydration, but this component starts with `theme` set to `'light'`; the `[theme]` effect therefore applies and persists light before the initialization effect's `setTheme(initialTheme)` update is reflected, then applies dark again. This can cause a theme flash and an unnecessary overwrite of the user's saved preference during startup.</comment>
<file context>
@@ -0,0 +1,56 @@
+}
+
+export default function ThemeToggle() {
+ const [theme, setTheme] = useState<Theme>('light');
+
+ useEffect(() => {
</file context>
| <div className="min-h-screen bg-gradient-to-br from-blue-50 to-indigo-100 dark:from-slate-900 dark:to-slate-950 flex items-center justify-center p-4"> | ||
| <div className="max-w-4xl w-full"> | ||
| <div className="bg-white rounded-2xl shadow-xl overflow-hidden"> | ||
| <div className="bg-white dark:bg-slate-900 rounded-2xl shadow-xl overflow-hidden border border-gray-200 dark:border-slate-800"> |
There was a problem hiding this comment.
P3: The per-element dark background utilities added here (dark:bg-slate-900/800, dark:bg-amber-950/40, dark:bg-red-950/*, dark:bg-indigo-950/70, and the dark:from/to-slate gradient on the demo panel) are overridden by the broad !important rules in app/globals.css (e.g. html.dark [class*='bg-white'], [class*='bg-amber-50'], [class*='bg-red-50'], [class*='bg-indigo-100'], [class*='bg-gradient-to-br']) that match the base light class still on the same element. They therefore never take effect; dark backgrounds come from the global overrides instead. Either drop these redundant dark:bg classes or make the dark mode styling consistent (e.g. remove the conflicting global overrides) so the intent isn't misleading.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/app/login/page.tsx, line 106:
<comment>The per-element dark background utilities added here (dark:bg-slate-900/800, dark:bg-amber-950/40, dark:bg-red-950/*, dark:bg-indigo-950/70, and the dark:from/to-slate gradient on the demo panel) are overridden by the broad `!important` rules in app/globals.css (e.g. `html.dark [class*='bg-white']`, `[class*='bg-amber-50']`, `[class*='bg-red-50']`, `[class*='bg-indigo-100']`, `[class*='bg-gradient-to-br']`) that match the base light class still on the same element. They therefore never take effect; dark backgrounds come from the global overrides instead. Either drop these redundant dark:bg classes or make the dark mode styling consistent (e.g. remove the conflicting global overrides) so the intent isn't misleading.</comment>
<file context>
@@ -101,24 +101,24 @@ function LoginPageContent() {
+ <div className="min-h-screen bg-gradient-to-br from-blue-50 to-indigo-100 dark:from-slate-900 dark:to-slate-950 flex items-center justify-center p-4">
<div className="max-w-4xl w-full">
- <div className="bg-white rounded-2xl shadow-xl overflow-hidden">
+ <div className="bg-white dark:bg-slate-900 rounded-2xl shadow-xl overflow-hidden border border-gray-200 dark:border-slate-800">
<div className={`grid ${isDemoMode ? 'md:grid-cols-2' : ''}`}>
{/* Login Form */}
</file context>
|
Thanks for the dark-mode work. I re-reviewed the latest head
Minor accessibility follow-up: add Please re-request review after these fixes, keeping the change scoped to issue #140. |
There was a problem hiding this comment.
2 issues found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/app/globals.css">
<violation number="1" location="frontend/app/globals.css:45">
P2: Dark-mode hover states now use light Tailwind colors: these exact-class selectors match only unprefixed utility tokens, so `hover:bg-*` and `hover:text-*` classes are no longer remapped and controls can flash light or near-black against the dark surface. Add explicit dark hover utilities in the affected components or exact escaped variant selectors in this stylesheet.</violation>
</file>
<file name="frontend/components/__tests__/ThemeToggle.test.tsx">
<violation number="1" location="frontend/components/__tests__/ThemeToggle.test.tsx:61">
P3: This test's title claims it verifies a "stable initial render" when the document is bootstrapped to dark, but it actually asserts the post-effect DOM state (☀️ Light / aria-pressed=true), which is the same resolved-dark state checked by the other tests. Because ThemeToggle starts with theme='light', its first paint is always '🌙 Dark' and only flips to dark after the mount effect; this test would still pass if the initial render flickered to light, so it gives false confidence about the theme-flash behavior it's named to protect against. Consider asserting the actual first-paint state (or resolving this by having the component derive the initial theme instead of starting at 'light') so the test matches its intent.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| html.dark .bg-white, | ||
| html.dark .bg-slate-50, | ||
| html.dark .bg-slate-100, | ||
| html.dark .bg-gray-50, |
There was a problem hiding this comment.
P2: Dark-mode hover states now use light Tailwind colors: these exact-class selectors match only unprefixed utility tokens, so hover:bg-* and hover:text-* classes are no longer remapped and controls can flash light or near-black against the dark surface. Add explicit dark hover utilities in the affected components or exact escaped variant selectors in this stylesheet.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/app/globals.css, line 45:
<comment>Dark-mode hover states now use light Tailwind colors: these exact-class selectors match only unprefixed utility tokens, so `hover:bg-*` and `hover:text-*` classes are no longer remapped and controls can flash light or near-black against the dark surface. Add explicit dark hover utilities in the affected components or exact escaped variant selectors in this stylesheet.</comment>
<file context>
@@ -39,92 +39,64 @@ html.dark body {
+html.dark .bg-white,
+html.dark .bg-slate-50,
+html.dark .bg-slate-100,
+html.dark .bg-gray-50,
+html.dark .bg-gray-100 {
background-color: var(--surface) !important;
</file context>
| }); | ||
| }); | ||
|
|
||
| it('uses a stable initial render even when the document is already bootstrapped to dark mode', () => { |
There was a problem hiding this comment.
P3: This test's title claims it verifies a "stable initial render" when the document is bootstrapped to dark, but it actually asserts the post-effect DOM state (☀️ Light / aria-pressed=true), which is the same resolved-dark state checked by the other tests. Because ThemeToggle starts with theme='light', its first paint is always '🌙 Dark' and only flips to dark after the mount effect; this test would still pass if the initial render flickered to light, so it gives false confidence about the theme-flash behavior it's named to protect against. Consider asserting the actual first-paint state (or resolving this by having the component derive the initial theme instead of starting at 'light') so the test matches its intent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/components/__tests__/ThemeToggle.test.tsx, line 61:
<comment>This test's title claims it verifies a "stable initial render" when the document is bootstrapped to dark, but it actually asserts the post-effect DOM state (☀️ Light / aria-pressed=true), which is the same resolved-dark state checked by the other tests. Because ThemeToggle starts with theme='light', its first paint is always '🌙 Dark' and only flips to dark after the mount effect; this test would still pass if the initial render flickered to light, so it gives false confidence about the theme-flash behavior it's named to protect against. Consider asserting the actual first-paint state (or resolving this by having the component derive the initial theme instead of starting at 'light') so the test matches its intent.</comment>
<file context>
@@ -0,0 +1,100 @@
+ });
+ });
+
+ it('uses a stable initial render even when the document is already bootstrapped to dark mode', () => {
+ document.documentElement.setAttribute('data-theme', 'dark');
+ document.documentElement.classList.add('dark');
</file context>
2 participants
Summary by cubic
Adds a theme toggle with persisted preference and server-safe initial theme to deliver a consistent dark mode across the app. Updates login page and global styles for better contrast and polish in dark mode.
ThemeToggle(bottom-right) to switch light/dark, storing preference inlocalStorageand syncingdata-themeandhtml.dark.layout.tsxand enabledsuppressHydrationWarningto set the theme before hydration and prevent flash.darkMode: 'class'and expandedglobals.csswith variables and dark overrides for surfaces, text, borders, shadows, gradients, inputs, and selection.ThemeTogglecovering bootstrapped state, system preference fallback, persistence, and storage errors.Written for commit 4f767a5. Summary will update on new commits.