Copilot commented on code in PR #148:
URL: https://github.com/apache/datafusion-site/pull/148#discussion_r2769565940
##########
content/js/darkmode.js:
##########
@@ -0,0 +1,96 @@
+/**
+ * Dark Mode Toggle Functionality for Apache DataFusion Blog
+ */
+
+(function() {
+ 'use strict';
+
+ // Constants
+ const THEME_KEY = 'datafusion-theme';
+ const THEME_DARK = 'dark';
+ const THEME_LIGHT = 'light';
+
+ /**
+ * Get the current theme from localStorage or system preference
+ */
+ 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) {
+ // Use requestAnimationFrame for smoother transition
+ requestAnimationFrame(() => {
+ if (theme === THEME_DARK) {
+ document.documentElement.setAttribute('data-theme',
THEME_DARK);
+ } else {
+ document.documentElement.removeAttribute('data-theme');
+ }
+ });
+ localStorage.setItem(THEME_KEY, theme);
+ }
Review Comment:
The localStorage operations lack error handling, which could cause the dark
mode feature to fail in private browsing mode or when storage quota is
exceeded. Consider wrapping localStorage.getItem and localStorage.setItem calls
in try-catch blocks to gracefully handle these scenarios. When localStorage is
unavailable, the theme could still work for the current session using the
system preference, similar to the approach in giscus-consent.js (lines 86, 91,
95-100).
##########
content/theme/templates/menu.html:
##########
@@ -15,6 +15,10 @@
<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">
Review Comment:
The theme toggle button is missing an explicit type attribute. While
browsers default button elements to type="button", it's best practice to
explicitly specify it to avoid any potential issues. Add type="button" to the
button element to make the intent clear and prevent accidental form submission
if this button is ever nested within a form.
```suggestion
<button id="theme-toggle" class="theme-toggle" type="button"
aria-label="Toggle dark mode" title="Toggle dark mode">
```
##########
content/js/darkmode.js:
##########
@@ -0,0 +1,96 @@
+/**
+ * Dark Mode Toggle Functionality for Apache DataFusion Blog
+ */
+
+(function() {
+ 'use strict';
+
+ // Constants
+ const THEME_KEY = 'datafusion-theme';
+ const THEME_DARK = 'dark';
+ const THEME_LIGHT = 'light';
+
+ /**
+ * Get the current theme from localStorage or system preference
+ */
+ 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) {
+ // Use requestAnimationFrame for smoother transition
+ requestAnimationFrame(() => {
+ if (theme === THEME_DARK) {
+ document.documentElement.setAttribute('data-theme',
THEME_DARK);
+ } else {
+ document.documentElement.removeAttribute('data-theme');
+ }
+ });
+ localStorage.setItem(THEME_KEY, theme);
+ }
+
+ /**
+ * Toggle between light and dark themes
+ */
+ 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());
Review Comment:
The initialization logic always saves a theme to localStorage on line 61,
even when the user hasn't explicitly chosen a theme. This means the system
theme change listener on lines 87-91 will never trigger after the first page
load, because getStoredTheme() will always return a value.
This prevents the theme from automatically updating when the user changes
their system preference after visiting the site. Consider modifying the
initialization to only apply the theme without saving to localStorage, and only
save to localStorage when the user explicitly clicks the toggle button. This
way, users who haven't made an explicit choice will continue to follow their
system preference.
##########
content/css/darkmode.css:
##########
@@ -0,0 +1,682 @@
+/* Dark Mode Styles for Apache DataFusion Blog */
+
+/* Performance optimizations for theme switching */
+html {
+ /* Enable GPU acceleration for smoother transitions */
+ -webkit-font-smoothing: antialiased;
+ -moz-osx-font-smoothing: grayscale;
+}
+
+/* Root variables for light mode (default) */
+:root {
+ --bg-primary: #ffffff;
+ --bg-secondary: #f8f9fa;
+ --bg-tertiary: #e9ecef;
+ --text-primary: #212529;
+ --text-secondary: #6c757d;
+ --text-muted: #868e96;
+ --border-color: #dee2e6;
+ --callout-bg: #f8f9fa;
+ --code-bg: #f8f9fa;
+ --link-color: #0d6efd;
+ --link-hover: #0a58ca;
+ --toc-border: #eee;
+ --shadow: rgba(0, 0, 0, 0.1);
+ --accent-color: #0d6efd;
+}
+
+/* Dark mode variables - Modern, high contrast palette */
+[data-theme="dark"] {
+ --bg-primary: #0d1117;
+ --bg-secondary: #161b22;
+ --bg-tertiary: #1c2128;
+ --text-primary: #e6edf3;
+ --text-secondary: #8b949e;
+ --text-muted: #6e7681;
+ --border-color: #30363d;
+ --callout-bg: #161b22;
+ --code-bg: #0d1117;
+ --link-color: #58a6ff;
+ --link-hover: #79c0ff;
+ --toc-border: #30363d;
+ --shadow: rgba(0, 0, 0, 0.5);
+ --accent-color: #58a6ff;
+}
+
+/* Apply dark mode styles */
+[data-theme="dark"] body {
+ background-color: var(--bg-primary);
+ color: var(--text-primary);
+ /* Optimize rendering performance */
+ will-change: background-color, color;
Review Comment:
The will-change property is set on the body element for background-color and
color. The will-change property should be used sparingly and typically only on
elements that are actively being animated, as it can consume additional memory.
Since the theme only changes occasionally (when the user clicks the toggle
button), this optimization may not provide significant benefits and could
potentially increase memory usage. Consider removing this property or only
applying it during the actual transition using JavaScript.
```suggestion
```
##########
_typos.toml:
##########
@@ -0,0 +1,53 @@
+# 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
+
+# Ignore patterns for strings that contain false positives
+[default]
+extend-ignore-re = [
+ "chloro-pn", # GitHub username
+ "2010YOUY01", # GitHub username
Review Comment:
Trailing whitespace detected at the end of this line. Consider removing it
to maintain consistent code formatting.
##########
content/js/darkmode.js:
##########
@@ -0,0 +1,96 @@
+/**
+ * Dark Mode Toggle Functionality for Apache DataFusion Blog
+ */
+
+(function() {
+ 'use strict';
+
+ // Constants
+ const THEME_KEY = 'datafusion-theme';
+ const THEME_DARK = 'dark';
+ const THEME_LIGHT = 'light';
+
+ /**
+ * Get the current theme from localStorage or system preference
+ */
+ 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) {
+ // Use requestAnimationFrame for smoother transition
+ requestAnimationFrame(() => {
+ if (theme === THEME_DARK) {
+ document.documentElement.setAttribute('data-theme',
THEME_DARK);
+ } else {
+ document.documentElement.removeAttribute('data-theme');
+ }
+ });
+ localStorage.setItem(THEME_KEY, theme);
+ }
+
+ /**
+ * Toggle between light and dark themes
+ */
+ 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();
+ });
+ }
+ }
+
+ /**
+ * Listen for system theme changes
+ */
+ window.matchMedia('(prefers-color-scheme:
dark)').addEventListener('change', function(e) {
+ if (!getStoredTheme()) {
+ setTheme(e.matches ? THEME_DARK : THEME_LIGHT);
+ }
+ });
+
+ // Initialize theme immediately
+ initTheme();
+
+})();
Review Comment:
The dark mode implementation does not integrate with the Giscus comments
system. The giscus-consent.js file (line 51) hardcodes the theme to 'light'.
When users switch to dark mode, the comments section will remain in light
theme, creating a jarring visual inconsistency.
Consider updating the dark mode implementation to also switch the Giscus
theme. This would require:
1. Exporting a function from darkmode.js that returns the current theme
2. Modifying giscus-consent.js to use the current theme when injecting Giscus
3. Adding a listener to re-inject or update Giscus when the theme changes
Giscus supports themes including 'dark', 'dark_dimmed',
'dark_high_contrast', and 'transparent_dark' that would be appropriate for dark
mode.
##########
_typos.toml:
##########
@@ -0,0 +1,53 @@
+# 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
+
+# Ignore patterns for strings that contain false positives
+[default]
+extend-ignore-re = [
+ "chloro-pn", # GitHub username
+ "2010YOUY01", # GitHub username
+ "RinChanNOWWW", # GitHub username
+ "ANDed", # Technical term (ANDed predicates)
+ "NDJson", # Data format name
+ "efully express fo\\|", # TPC-H dataset artifact (truncated data)
+]
+
+# Custom dictionary for technical terms (whole word matching only)
+[default.extend-words]
+# Add any false positives here if needed
+
+# GitHub usernames (keys must be lowercase for case-insensitive matching)
+youy = "youy"
+
+# Product/Service names
+vertica = "vertica"
+
+# Personal names
+parth = "parth"
+authers = "authers"
+
+# Technical terms
+ndjson = "ndjson"
+anded = "anded"
+rin = "rin"
+
+# Add any false positives here if needed
+
+
Review Comment:
Duplicate comment found. The comment "Add any false positives here if
needed" appears on line 34 and again on line 51. Consider removing one of these
duplicate comments to keep the configuration file clean.
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
