Copilot commented on code in PR #156:
URL: https://github.com/apache/datafusion-site/pull/156#discussion_r2935412530
##########
content/theme/templates/menu.html:
##########
@@ -15,6 +15,10 @@
<a class="nav-link" href="/blog/feed.xml">RSS</a>
</li>
</ul>
+ <button id="dark-mode-toggle" type="button"
class="dark-mode-toggle" aria-label="Toggle dark mode" aria-pressed="false"
title="Toggle dark mode">
+ <span class="sun-icon" aria-hidden="true">☀</span>
+ <span class="moon-icon" aria-hidden="true">☾</span>
+ </button>
Review Comment:
These changes introduce a dark-mode feature (toggle button + assets), but
the PR title/description and linked issue are about the mobile hamburger menu
not responding. Either update the PR description to cover the dark-mode
addition (and rationale), or split dark-mode into a separate PR so the
hamburger fix can be reviewed/merged independently.
##########
content/js/dark-mode.js:
##########
@@ -0,0 +1,42 @@
+(function() {
+ 'use strict';
+
+ const root = document.documentElement;
+
+ function getTheme() {
+ return localStorage.getItem('theme') || 'light';
+ }
+
+ function setButtonState(theme) {
+ const toggleButton = document.getElementById('dark-mode-toggle');
+ if (toggleButton) {
+ toggleButton.setAttribute('aria-pressed', theme === 'dark' ?
'true' : 'false');
+ }
+ }
+
+ function applyTheme(theme) {
+ root.setAttribute('data-theme', theme);
+ localStorage.setItem('theme', theme);
+ setButtonState(theme);
+ }
Review Comment:
`localStorage` access can throw in some environments (e.g., storage disabled
/ privacy modes). Elsewhere in the repo (e.g., giscus-consent.js) storage
reads/writes are wrapped in try/catch; here an exception would prevent theme
initialization and button state updates. Consider guarding `getTheme()` /
`applyTheme()` with try/catch and falling back to a default theme when storage
is unavailable.
##########
content/js/mobile-nav.js:
##########
@@ -0,0 +1,33 @@
+(function () {
+ function initMobileNav() {
+ const toggler =
document.querySelector('.navbar-toggler[data-bs-target]');
+ if (!toggler) {
+ return;
+ }
+
+ const targetSelector = toggler.getAttribute('data-bs-target');
+ if (!targetSelector) {
+ return;
+ }
+
+ const collapseElement = document.querySelector(targetSelector);
+ if (!collapseElement) {
+ return;
+ }
+
+ toggler.addEventListener('click', function () {
+ const isExpanded = toggler.getAttribute('aria-expanded') ===
'true';
+ const nextExpanded = !isExpanded;
+
+ toggler.setAttribute('aria-expanded', String(nextExpanded));
+ toggler.classList.toggle('collapsed', !nextExpanded);
+ collapseElement.classList.toggle('show', nextExpanded);
+ });
Review Comment:
This click handler will run in addition to Bootstrap’s built-in collapse
handler (menu.html uses `data-bs-toggle="collapse"`, and
bootstrap.bundle.min.js is loaded). Since event listeners run in registration
order, Bootstrap will toggle the menu and update `aria-expanded`, then this
handler will immediately invert it again—effectively resulting in no visible
change on browsers where Bootstrap works. To avoid the double-toggle, either
disable Bootstrap’s data API for this button (e.g., remove `data-bs-toggle` in
init / use a different selector) or use Bootstrap’s Collapse API instead of
manually toggling classes, but ensure only one toggle path executes per click.
--
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]
