Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements dark mode support for the cycling website using Tailwind CSS's class-based dark mode system, CSS variables, and a theme toggle button. The implementation includes theme persistence via localStorage and respects system preferences as a fallback.
Key Changes
- Configured Tailwind CSS with
darkMode: 'class'to enable class-based dark mode - Added theme toggle button in header with sun/moon icons and JavaScript for theme switching
- Converted footer from dark-themed to light-themed with plans to support both modes via theme toggle
- Introduced CSS variables for colors and created global dark mode overrides
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 17 comments.
| File | Description |
|---|---|
tailwind.config.cjs |
Enabled class-based dark mode configuration |
src/components/Global/Header.astro |
Added theme toggle button with JavaScript for switching between light/dark modes and applied dark variants to header elements |
src/components/Global/Footer.astro |
Changed footer from dark to light theme but missing dark mode variants for proper theme support |
src/assets/styles/global.css |
Added CSS variables and extensive dark mode overrides using !important, which conflicts with Tailwind's preferred dark: variant approach |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (toggle) { | ||
| toggle.addEventListener('click', () => { | ||
| const isDark = document.documentElement.classList.contains('dark'); | ||
| const next = isDark ? 'light' : 'dark'; | ||
| try { localStorage.setItem('theme', next); } catch (e) {} | ||
| applyTheme(next); | ||
| }); | ||
| } |
There was a problem hiding this comment.
The theme toggle button should announce its current state to screen reader users. The current implementation only has a static aria-label="Toggle dark mode".
Consider using aria-pressed to indicate the toggle state:
if (toggle) {
const updateAriaPressed = () => {
const isDark = document.documentElement.classList.contains('dark');
toggle.setAttribute('aria-pressed', isDark ? 'true' : 'false');
};
updateAriaPressed(); // Set initial state
toggle.addEventListener('click', () => {
const isDark = document.documentElement.classList.contains('dark');
const next = isDark ? 'light' : 'dark';
try { localStorage.setItem('theme', next); } catch (e) {}
applyTheme(next);
updateAriaPressed();
});
}|
|
||
| <!-- Copyright --> | ||
| <div class="pt-8 mt-8 border-t border-gray-800 text-center text-gray-400 text-sm"> | ||
| <div class="pt-8 mt-8 border-t border-gray-200 text-center text-gray-600 text-sm"> |
There was a problem hiding this comment.
Copyright section lacks dark mode variants for text and border colors. The text-gray-600 and border-gray-200 will have poor contrast in dark mode.
Add dark mode variants:
<div class="pt-8 mt-8 border-t border-gray-200 dark:border-gray-700 text-center text-gray-600 dark:text-gray-400 text-sm">| <div class="pt-8 mt-8 border-t border-gray-200 text-center text-gray-600 text-sm"> | |
| <div class="pt-8 mt-8 border-t border-gray-200 dark:border-gray-700 text-center text-gray-600 dark:text-gray-400 text-sm"> |
|
|
||
| /* Header-specific: make the brand subtitle and icon readable in dark mode */ | ||
| .site-subtitle { /* keep light mode blue via tailwind utility on the element */ } | ||
| .site-icon { /* keep light mode color via tailwind utility on the element */ } |
There was a problem hiding this comment.
Empty CSS rulesets with only comments serve no purpose and add clutter. These selector declarations with empty blocks should be removed.
| .site-icon { /* keep light mode color via tailwind utility on the element */ } | |
| /* .site-icon: keep light mode color via tailwind utility on the element */ |
| --- | ||
|
|
||
| <footer class="bg-gray-900 text-white pt-16 pb-8"> | ||
| <footer class="bg-white pt-16 pb-8 footer-root"> |
There was a problem hiding this comment.
The footer changed from dark styling (bg-gray-900 text-white) to light styling (bg-white), but there are no dark mode variants added for the footer. This means the footer will remain light-colored in dark mode, creating an inconsistent user experience.
Add dark mode classes:
<footer class="bg-white dark:bg-slate-900 pt-16 pb-8 footer-root">Also consider adding dark variants to other footer elements that are now styled for light mode only.
| </g> | ||
| </svg> | ||
| <span class="text-primary-800">ギークのためのサイクリング</span> | ||
| <span class="text-primary-800 site-subtitle">ギークのためのサイクリング</span> |
There was a problem hiding this comment.
The site-subtitle class is added here but the element already has Tailwind's text-primary-800 class. The global CSS then overrides this with var(--text) in dark mode. This creates unnecessary indirection.
Consider using Tailwind's dark: variant directly instead:
<span class="text-primary-800 dark:text-gray-100">ギークのためのサイクリング</span>This would eliminate the need for the custom .site-subtitle class and CSS overrides in global.css (lines 97-99).
| <span class="text-primary-800 site-subtitle">ギークのためのサイクリング</span> | |
| <span class="text-primary-800 dark:text-gray-100">ギークのためのサイクリング</span> |
| .dark .bg-white { background-color: var(--card) !important; } | ||
| .dark .bg-gray-50 { background-color: var(--bg) !important; } | ||
| .dark .bg-gray-100 { background-color: var(--bg) !important; } | ||
| .dark .text-gray-900, .dark .text-gray-800, .dark .text-black { color: var(--text) !important; } | ||
| .dark .text-gray-700, .dark .text-gray-600, .dark .text-gray-500 { color: var(--muted) !important; } | ||
| .dark .border-gray-100, .dark .border-gray-200, .dark .border-gray-300 { border-color: var(--border) !important; } | ||
|
|
||
| /* Cards/button backgrounds that use white should look correct in dark */ | ||
| .dark .card { background-color: var(--card) !important; } | ||
| .dark .btn-outline { background-color: transparent !important; color: var(--text) !important; border-color: var(--border) !important; } | ||
|
|
||
| /* If components use subtle gray backgrounds, make them transparent on dark */ | ||
| .dark .bg-gray-200, .dark .bg-gray-300 { background-color: rgba(255,255,255,0.03) !important; } | ||
|
|
||
| /* Links / primary accents: keep bright colors unless specifically overridden */ | ||
| /* Add more overrides here if you encounter other utility classes that need dark variants */ |
There was a problem hiding this comment.
Using !important extensively throughout the dark mode overrides defeats the purpose of CSS specificity and makes future customization difficult. This approach is fragile and goes against CSS best practices.
Instead, prefer using Tailwind's built-in dark: variant system (already configured with darkMode: 'class') or use higher specificity selectors without !important. For example:
.dark .bg-white {
background-color: var(--card);
}This works because .dark .bg-white has higher specificity than .bg-white alone.
| .dark .site-icon { | ||
| filter: drop-shadow(0 1px 0 rgba(0,0,0,0.35)); | ||
| } |
There was a problem hiding this comment.
This duplicate .dark .site-icon selector was already defined at line 101. Consolidate these rules into a single selector block to avoid confusion and potential conflicts.
| footerNavs.map((group) => ( | ||
| <div> | ||
| <h4 class="text-lg font-bold mb-4 text-white">{group.title}</h4> | ||
| <h4 class="text-lg font-bold mb-4 text-gray-900">{group.title}</h4> |
There was a problem hiding this comment.
Footer heading colors lack dark mode variants. The text-gray-900 class will have poor contrast against a dark background if the footer is styled for dark mode.
Add dark mode variant:
<h4 class="text-lg font-bold mb-4 text-gray-900 dark:text-gray-100">{group.title}</h4>| <h4 class="text-lg font-bold mb-4 text-gray-900">{group.title}</h4> | |
| <h4 class="text-lg font-bold mb-4 text-gray-900 dark:text-gray-100">{group.title}</h4> |
| --- | ||
|
|
||
| <footer class="bg-gray-900 text-white pt-16 pb-8"> | ||
| <footer class="bg-white pt-16 pb-8 footer-root"> |
There was a problem hiding this comment.
The footer-root class is added but never defined or used in the CSS. Either define styles for this class or remove it to keep the code clean.
| <footer class="bg-white pt-16 pb-8 footer-root"> | |
| <footer class="bg-white pt-16 pb-8"> |
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| class="text-gray-400 hover:text-white transition-colors" | ||
| class="text-gray-600 hover:text-primary-600 transition-colors" |
There was a problem hiding this comment.
Social link colors lack dark mode variants. The text-gray-600 will have insufficient contrast against a dark footer background.
Add dark mode variant:
class="text-gray-600 dark:text-gray-400 hover:text-primary-600 dark:hover:text-primary-400 transition-colors"| class="text-gray-600 hover:text-primary-600 transition-colors" | |
| class="text-gray-600 dark:text-gray-400 hover:text-primary-600 dark:hover:text-primary-400 transition-colors" |
added darkmode