Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request adds dark mode support to the Apache DataFusion Blog website with a toggle button for switching between light and dark themes. The implementation includes a comprehensive dark mode color scheme, theme persistence via localStorage, and automatic detection of system theme preferences. Additionally, the PR introduces a typo-checking workflow and configuration, which appears to be unrelated to the primary dark mode feature.
Changes:
- Added dark mode toggle button in the navigation menu with animated sun/moon icons
- Implemented comprehensive dark mode styling with CSS custom properties for all site elements
- Added JavaScript for theme switching, persistence, and system preference detection
- Introduced typo-checking GitHub workflow and configuration (unrelated to dark mode)
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| content/theme/templates/styles.html | Added links to new darkmode.css stylesheet and darkmode.js script |
| content/theme/templates/menu.html | Added theme toggle button with sun/moon emoji icons |
| content/js/darkmode.js | Implements theme toggling, localStorage persistence, and system preference detection |
| content/css/darkmode.css | Comprehensive dark mode styling for all site elements with GitHub-inspired color scheme |
| _typos.toml | Configuration for typos spell checker (unrelated to dark mode) |
| .github/workflows/typo-check.yml | GitHub workflow for spell checking blog content (unrelated to dark mode) |
Comments suppressed due to low confidence (2)
.github/workflows/typo-check.yml:37
- The addition of typo-check workflow and _typos.toml configuration appears unrelated to the dark mode feature described in the PR title and issue #146. Consider separating this into a different PR focused on adding spell checking infrastructure, as it addresses a different concern and makes the PR harder to review. This would also make the git history clearer and allow for independent discussion of the two features.
# Licensed to the Apache Software Foundation (ASF) under one
# or more contributor license agreements. See the NOTICE file
# distributed with this work for additional information
# regarding copyright ownership. The ASF licenses this file
# to you under the Apache License, Version 2.0 (the
# "License"); you may not use this file except in compliance
# with the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing,
# software distributed under the License is distributed on an
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
name: Typo Check
on:
push:
branches:
- main
pull_request:
jobs:
typos:
name: Spell Check with Typos
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0
with:
persist-credentials: false
- uses: crate-ci/typos@40156d6074bf731adb169cfb8234954971dbc487 # v1.37.1
with:
files: ./content/blog/
_typos.toml:24
- The addition of typo-check configuration appears unrelated to the dark mode feature described in the PR title and issue #146. Consider separating this into a different PR focused on adding spell checking infrastructure, as it addresses a different concern and makes the PR harder to review. This would also make the git history clearer and allow for independent discussion of the two features.
# Licensed to the Apache Software Foundation (ASF) under one
# or more contributor license agreements. See the NOTICE file
# distributed with this work for additional information
# regarding copyright ownership. The ASF licenses this file
# to you under the Apache License, Version 2.0 (the
# "License"); you may not use this file except in compliance
# with the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing,
# software distributed under the License is distributed on an
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
# Configuration for typos spell checker
# https://github.com/crate-ci/typos
# Custom dictionary for technical terms
[default.extend-words]
# Add any false positives here if needed
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <a class="nav-link" href="/blog/feed.xml">RSS</a> | ||
| </li> | ||
| </ul> | ||
| <button id="theme-toggle" class="theme-toggle" aria-label="Toggle dark mode" title="Toggle dark mode"> |
There was a problem hiding this comment.
The button element should have an explicit type attribute. Add type="button" to prevent the button from inadvertently submitting forms if it's ever placed inside a form element.
| <button id="theme-toggle" class="theme-toggle" aria-label="Toggle dark mode" title="Toggle dark mode"> | |
| <button id="theme-toggle" class="theme-toggle" type="button" aria-label="Toggle dark mode" title="Toggle dark mode"> |
|
|
||
| # Custom dictionary for technical terms | ||
| [default.extend-words] | ||
| # Add any false positives here if needed |
There was a problem hiding this comment.
There is a trailing space at the end of this comment line. Remove the trailing space to maintain clean code formatting.
| # Add any false positives here if needed | |
| # Add any false positives here if needed |
|
|
||
| /* Navbar - ensure it looks good in dark mode */ | ||
| [data-theme="dark"] .navbar-dark { | ||
| background-color: #0d1117 !important; |
There was a problem hiding this comment.
For consistency with the rest of the CSS, consider using the CSS variable instead of the hardcoded color value. Replace #0d1117 with var(--bg-primary) to maintain consistency with the color scheme defined at the top of the file.
| background-color: #0d1117 !important; | |
| background-color: var(--bg-primary) !important; |
| function getStoredTheme() { | ||
| return localStorage.getItem(THEME_KEY); | ||
| } | ||
|
|
||
| /** | ||
| * Get the preferred theme based on system settings | ||
| */ | ||
| function getPreferredTheme() { | ||
| const storedTheme = getStoredTheme(); | ||
| if (storedTheme) { | ||
| return storedTheme; | ||
| } | ||
| // Check system preference | ||
| return window.matchMedia('(prefers-color-scheme: dark)').matches ? THEME_DARK : THEME_LIGHT; | ||
| } | ||
|
|
||
| /** | ||
| * Set the theme on the document | ||
| */ | ||
| function setTheme(theme) { | ||
| if (theme === THEME_DARK) { | ||
| document.documentElement.setAttribute('data-theme', THEME_DARK); | ||
| } else { | ||
| document.documentElement.removeAttribute('data-theme'); | ||
| } | ||
| localStorage.setItem(THEME_KEY, theme); |
There was a problem hiding this comment.
The localStorage access should be wrapped in try-catch blocks to handle cases where localStorage is disabled, unavailable (e.g., in private browsing mode), or when storage quota is exceeded. Without error handling, the entire script could fail silently, preventing the theme toggle functionality from working. Consider wrapping localStorage calls in try-catch blocks similar to how giscus-consent.js handles it.
|
|
||
| [data-theme="dark"] ::-webkit-scrollbar-thumb:hover { | ||
| background: var(--text-muted); | ||
| } |
There was a problem hiding this comment.
Add a blank line before this comment for better readability and consistency with the rest of the CSS file structure.
| } | |
| } |
| name: Spell Check with Typos | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0 |
There was a problem hiding this comment.
The version v5.0.0 of actions/checkout should be verified. As of January 2025, the latest major version was v4. If v5 has been released since then, this is correct. However, for consistency with other workflows in this repository (publish-site.yml and stage-site.yml), consider using the same version notation style. The other workflows use @v4 instead of pinned hashes. If pinned hashes are preferred for security, consider updating the other workflows as well for consistency.
| function toggleTheme() { | ||
| const currentTheme = getStoredTheme() || THEME_LIGHT; | ||
| const newTheme = currentTheme === THEME_DARK ? THEME_LIGHT : THEME_DARK; | ||
| setTheme(newTheme); | ||
| } | ||
|
|
||
| /** | ||
| * Initialize theme on page load | ||
| */ | ||
| function initTheme() { | ||
| // Apply theme immediately to prevent flash | ||
| setTheme(getPreferredTheme()); | ||
|
|
||
| // Add event listener to toggle button when DOM is ready | ||
| if (document.readyState === 'loading') { | ||
| document.addEventListener('DOMContentLoaded', attachToggleListener); | ||
| } else { | ||
| attachToggleListener(); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Attach click event listener to theme toggle button | ||
| */ | ||
| function attachToggleListener() { | ||
| const toggleButton = document.getElementById('theme-toggle'); | ||
| if (toggleButton) { | ||
| toggleButton.addEventListener('click', function(e) { | ||
| e.preventDefault(); | ||
| toggleTheme(); | ||
| }); |
There was a problem hiding this comment.
The theme toggle button should indicate its current state for screen reader users. Consider updating the aria-label dynamically to indicate the current state (e.g., "Enable dark mode" vs "Enable light mode") or add an aria-pressed attribute that is toggled when the theme changes. This would improve accessibility by letting screen reader users know the current theme state and what clicking the button will do.
| <span class="icon-sun">☀️</span> | ||
| <span class="icon-moon">🌙</span> |
There was a problem hiding this comment.
Using emoji characters (☀️ and 🌙) for the toggle icons may cause inconsistent rendering across different browsers and operating systems. Consider using SVG icons or icon fonts (like the FontAwesome already included in the project) for more consistent cross-platform appearance and better control over styling. Additionally, the emoji may be announced by screen readers, which could be confusing since the button already has an aria-label.
| <span class="icon-sun">☀️</span> | |
| <span class="icon-moon">🌙</span> | |
| <i class="icon-sun fa-regular fa-sun" aria-hidden="true"></i> | |
| <i class="icon-moon fa-regular fa-moon" aria-hidden="true"></i> |
Added dark mode support and toggle button for changing the mode (eg. ligh,dark)
also improve readability by adding some extra line of CSS
fixed issue : #146