Implement light/dark theme toggle with CSS tokens and chart theming by alvin000009238 · Pull Request #91 · alvin000009238/clhs_score

alvin000009238 · 2026-03-31T04:57:46Z

No description provided.

Restructure CI workflows for Docker deployment and add styles

…heme-toggle feat: add accessible light theme and header theme toggle

…heme-toggle-harb4d Add light/dark theme with toggle, CSS token refactor and theme persistence

…heme-toggle-ggawno Add light/dark theme support with toggle and CSS design tokens

…heme-toggle-zya5rs Add light/dark theme support with toggle and Chart.js theming

…heme-toggle-p94oog Add theme system and toggle (light/dark) with CSS tokens and chart theming

…heme-toggle-efl5ta Add light/dark theme support with toggle, CSS tokens, and Chart.js theme sync

…rk-mode-icons Update light/dark theme toggle icons

gemini-code-assist

Code Review

This pull request implements a theme switching system, introducing a light mode alongside the existing dark mode. Key changes include refactoring CSS to use variables, adding a theme toggle component with local storage persistence, and ensuring chart instances update dynamically via a new theme change event. Feedback suggests optimizing performance by caching the theme palette instead of repeatedly calling getComputedStyle during chart updates. Additionally, it was noted that the grid and gridSubtle palette properties currently map to the same CSS variable, which prevents the intended visual distinction between different grid line intensities.

gemini-code-assist · 2026-03-31T04:59:20Z

    const radarCtx = radarCanvas?.getContext('2d');
    if (!radarCtx) return;

+    const palette = getChartThemePalette();


Calling getChartThemePalette() here (and again in updateBarChart) results in redundant getComputedStyle calls during every chart update. Since generateCharts calls both update functions, it would be more efficient to fetch the palette once in generateCharts and pass it as an argument, or cache the palette object and only refresh it when a themechange event occurs.

gemini-code-assist · 2026-03-31T04:59:20Z

+        grid: readVar('--color-border-subtle', 'rgba(148, 163, 184, 0.22)'),
+        gridSubtle: readVar('--color-border-subtle', 'rgba(148, 163, 184, 0.16)')


Both grid and gridSubtle are currently mapped to the same CSS variable --color-border-subtle. This makes them identical in practice, which ignores the intended difference in transparency (0.22 vs 0.16 fallback). If the bar chart is intended to have more subtle grid lines than the radar chart, you should define a separate CSS variable (e.g., --color-border-extra-subtle) or apply an opacity modifier in JavaScript.

Copilot

Pull request overview

Adds a user-facing light/dark theme toggle using CSS tokens, and updates Chart.js rendering to follow the active theme.

Changes:

Introduce theme persistence + toggle UI and wire it into initial app boot.
Expand CSS token system to support light theme (including overlays/focus/selection) and adjust related component styles.
Make existing charts re-theme on a themechange event and use token-derived palette values.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
public/index.html	Adds theme toggle button + an early theme init script; updates hashed asset refs and a small label punctuation tweak.
frontend/theme.js	New module to read/write theme from localStorage, update DOM/theme button state, and emit `themechange`.
frontend/main.js	Initializes the theme toggle on DOMContentLoaded.
frontend/charts.js	Reads CSS variables for chart palette and re-applies colors to existing charts on theme changes.
frontend/styles/tokens.css	Adds light theme token overrides and introduces overlay/focus/selection variables; sets `color-scheme`.
frontend/styles/utilities.css	Styles the new theme toggle button.
frontend/styles/header.css	Adds `.header-actions` styling and tweaks button sizing/wrapping behavior.
frontend/styles/responsive.css	Ensures header action layout behaves on smaller viewports.
frontend/styles/onboarding.css	Replaces hardcoded overlay/highlight colors with theme tokens.
frontend/styles/motion.css	Uses token for selection highlight color.
frontend/styles/modal.css	Uses tokens for modal overlay and focus ring.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-31T05:01:31Z

+    <script>
+        (() => {
+            const storedTheme = localStorage.getItem('grades-theme');
+            if (storedTheme === 'light') {
+                document.documentElement.setAttribute('data-theme', 'light');
+            }
+        })();
+    </script>


The new inline <script> theme init block will be blocked by the current CSP: the page sets script-src-elem 'self' ... without 'unsafe-inline' or a nonce/hash, so inline script elements are disallowed even though script-src includes 'unsafe-inline'. Consider moving this logic into an external script from 'self' (e.g., a small Vite entry) or allow just this block via a CSP nonce/hash (preferred over loosening script-src-elem).

Suggested change

<script>

(() => {

const storedTheme = localStorage.getItem('grades-theme');

if (storedTheme === 'light') {

document.documentElement.setAttribute('data-theme', 'light');

}

})();

</script>

Copilot · 2026-03-31T05:01:32Z

+const STORAGE_KEY = 'grades-theme';
+
+function getInitialTheme() {
+    const storedTheme = localStorage.getItem(STORAGE_KEY);
+    if (storedTheme === 'light' || storedTheme === 'dark') return storedTheme;
+    return 'dark';
+}
+
+function applyTheme(theme) {
+    const root = document.documentElement;
+    if (theme === 'light') {
+        root.setAttribute('data-theme', 'light');
+    } else {
+        root.removeAttribute('data-theme');
+    }
+
+    const toggleBtn = document.getElementById('themeToggleBtn');
+    const sunIcon = toggleBtn?.querySelector('.theme-icon-sun');
+    const moonIcon = toggleBtn?.querySelector('.theme-icon-moon');
+
+    if (sunIcon && moonIcon) {
+        const isLight = theme === 'light';
+        sunIcon.classList.toggle('hidden', !isLight);
+        moonIcon.classList.toggle('hidden', isLight);
+    }
+
+    if (toggleBtn) {
+        const nextThemeLabel = theme === 'light' ? '深色' : '淺色';
+        toggleBtn.setAttribute('aria-label', `切換至${nextThemeLabel}模式`);
+        toggleBtn.setAttribute('title', `切換至${nextThemeLabel}模式`);
+    }
+
+    document.dispatchEvent(new CustomEvent('themechange', {
+        detail: { theme }
+    }));
+}
+
+export function setupThemeToggle() {
+    const toggleBtn = document.getElementById('themeToggleBtn');
+    if (!toggleBtn) return;
+
+    let currentTheme = getInitialTheme();
+    applyTheme(currentTheme);
+
+    toggleBtn.addEventListener('click', () => {
+        currentTheme = currentTheme === 'light' ? 'dark' : 'light';
+        applyTheme(currentTheme);
+        localStorage.setItem(STORAGE_KEY, currentTheme);
+    });
+}


New theme behavior (localStorage-backed theme selection, DOM attribute updates, and themechange event dispatch) isn’t covered by frontend unit tests. Since the repo already has Node-based frontend tests, please add tests to validate initial theme selection (stored/invalid), DOM updates (data-theme, aria-label/title, icon visibility), and that a themechange event is dispatched on toggle.

…nline-theme-init-script Make theme init CSP-safe, optimize chart palette usage, add extra subtle grid token and tests

…ript

…nline-theme-init-script Fix chart palette staleness and Node 20-safe theme tests

Copilot

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-31T06:28:19Z

+    const storedTheme = localStorage.getItem('grades-theme');
+    if (storedTheme === 'light') {
+        document.documentElement.setAttribute('data-theme', 'light');


theme-init.js 直接讀取 localStorage；在某些瀏覽器/隱私設定（封鎖儲存空間、第三方環境、Safari 特定模式）下存取 localStorage 可能丟出 SecurityError，造成此段 script 在 head 早期就報錯。建議用 try/catch 包起來，並在失敗時安靜跳過（保持預設深色），避免影響首屏與後續腳本。

Suggested change

const storedTheme = localStorage.getItem('grades-theme');

if (storedTheme === 'light') {

document.documentElement.setAttribute('data-theme', 'light');

try {

const storedTheme = localStorage.getItem('grades-theme');

if (storedTheme === 'light') {

document.documentElement.setAttribute('data-theme', 'light');

}

} catch (_e) {

// Access to localStorage may be blocked (e.g., privacy settings, third-party context).

// In that case, silently fall back to the default theme.

Copilot · 2026-03-31T06:28:19Z

    const labels = subjects.map((subject) => shortenName(subject.SubjectName));
    const myScores = subjects.map((subject) => subject.scoreValue ?? getNumericScore(subject.ScoreDisplay, subject.Score));
    const avgScores = subjects.map((subject) => subject.classAvgValue ?? getNumericScore(subject.ClassAVGScoreDisplay, subject.ClassAVGScore));
+    const palette = getChartThemePalette();


generateCharts() 內的 const palette = getChartThemePalette(); 目前沒有被使用，反而每次呼叫都會額外觸發一次 getComputedStyle()（成本不低）。建議移除這個未使用的變數，或如果原意是要在載入 Chart.js 前先快照當下主題，請改成實際使用（例如初始化 cachedChartPalette）。

Suggested change

const palette = getChartThemePalette();

Copilot · 2026-03-31T06:28:20Z

+export function __setChartInstancesForTest(instances = {}) {
+    if (Object.hasOwn(instances, 'radar')) radarChartInstance = instances.radar;
+    if (Object.hasOwn(instances, 'bar')) barChartInstance = instances.bar;
+}
+


__setChartInstancesForTest 目前會被打包成正式輸出 API，且專案其他模組看起來沒有類似的 test-only export 慣例。建議將此測試注入點改為在測試環境才可用（例如用條件式匯出/掛到全域測試鉤子/或把 chart instance 存取抽到可注入的依賴），避免未來被誤用成公開介面而增加維護負擔。

Suggested change

export function __setChartInstancesForTest(instances = {}) {

if (Object.hasOwn(instances, 'radar')) radarChartInstance = instances.radar;

if (Object.hasOwn(instances, 'bar')) barChartInstance = instances.bar;

}

function __setChartInstancesForTest(instances = {}) {

if (Object.hasOwn(instances, 'radar')) radarChartInstance = instances.radar;

if (Object.hasOwn(instances, 'bar')) barChartInstance = instances.bar;

}

if (typeof process !== 'undefined' && process.env && process.env.NODE_ENV === 'test') {

// Expose test-only hook via a global in test environments without making it a public export.

// This avoids leaking a test helper into the formal production API surface.

// eslint-disable-next-line no-undef

globalThis.__setChartInstancesForTest = __setChartInstancesForTest;

}

Copilot · 2026-03-31T06:28:20Z

-                    <div class="flex-center-gap-8">
+                    <div class="flex-center-gap-8 header-actions">
+                        <button class="icon-btn theme-toggle-btn" id="themeToggleBtn" type="button" aria-label="切換至淺色模式"
+                            title="切換主題">


theme toggle 按鈕初始狀態下 aria-label 與 title 的語意不一致（aria-label 是「切換至淺色模式」，title 是「切換主題」），而 theme.js 之後會把兩者都改成「切換至X模式」。建議在 HTML 初始值就讓 title 與 aria-label 一致，避免在 JS 尚未執行前 tooltip/輔助技術讀到不同訊息。

Suggested change

title="切換主題">

title="切換至淺色模式">

…ript-i8s1np

…nline-theme-init-script-i8s1np Cache chart theme palette, add theme-init, expose chart test hooks, and add theme tests

alvin000009238 added 21 commits March 31, 2026 10:03

Merge pull request #83 from alvin000009238/main

bed22cd

Restructure CI workflows for Docker deployment and add styles

feat: add accessible light theme toggle

a2ef854

Merge pull request #84 from alvin000009238/codex/add-light-mode-and-t…

8043951

…heme-toggle feat: add accessible light theme and header theme toggle

fix: prevent header action text clipping in light theme

615b603

Merge branch 'dev' into codex/add-light-mode-and-theme-toggle-harb4d

b5e66f6

Merge pull request #85 from alvin000009238/codex/add-light-mode-and-t…

8ec25fa

…heme-toggle-harb4d Add light/dark theme with toggle, CSS token refactor and theme persistence

fix: compact header actions to reduce space usage

61ddf8f

Merge branch 'dev' into codex/add-light-mode-and-theme-toggle-ggawno

ea82ecd

Merge pull request #86 from alvin000009238/codex/add-light-mode-and-t…

176f982

…heme-toggle-ggawno Add light/dark theme support with toggle and CSS design tokens

fix: improve chart text contrast across theme changes

a8bd092

Merge branch 'dev' into codex/add-light-mode-and-theme-toggle-zya5rs

1a786e6

Merge pull request #87 from alvin000009238/codex/add-light-mode-and-t…

d49917d

…heme-toggle-zya5rs Add light/dark theme support with toggle and Chart.js theming

fix: use manual dark default theme and md3 toggle icons

5c2e031

Merge branch 'dev' into codex/add-light-mode-and-theme-toggle-p94oog

285e939

Merge pull request #88 from alvin000009238/codex/add-light-mode-and-t…

a288985

…heme-toggle-p94oog Add theme system and toggle (light/dark) with CSS tokens and chart theming

fix: swap theme toggle to md3-style svg icons

45abbf6

Merge branch 'dev' into codex/add-light-mode-and-theme-toggle-efl5ta

b666c85

Merge pull request #89 from alvin000009238/codex/add-light-mode-and-t…

52bc78a

…heme-toggle-efl5ta Add light/dark theme support with toggle, CSS tokens, and Chart.js theme sync

Update light and dark mode toggle SVG icons

2e9d751

Merge pull request #90 from alvin000009238/codex/replace-light-and-da…

aa30858

…rk-mode-icons Update light/dark theme toggle icons

Fix typo in heading from 'vs' to 'vs.'

974f03f

Copilot AI review requested due to automatic review settings March 31, 2026 04:57

Copilot started reviewing on behalf of alvin000009238 March 31, 2026 04:58 View session

gemini-code-assist Bot reviewed Mar 31, 2026

View reviewed changes

Copilot AI reviewed Mar 31, 2026

View reviewed changes

alvin000009238 added 5 commits March 31, 2026 13:15

Fix theme init CSP and improve theme/chart test coverage

5099d53

Merge pull request #92 from alvin000009238/codex/fix-csp-issue-with-i…

ede308a

…nline-theme-init-script Make theme init CSP-safe, optimize chart palette usage, add extra subtle grid token and tests

Fix chart palette staleness and stabilize theme tests

4f84465

Merge branch 'dev' into codex/fix-csp-issue-with-inline-theme-init-sc…

007dfab

…ript

Merge pull request #93 from alvin000009238/codex/fix-csp-issue-with-i…

6f9ee92

…nline-theme-init-script Fix chart palette staleness and Node 20-safe theme tests

Copilot AI review requested due to automatic review settings March 31, 2026 06:24

Copilot started reviewing on behalf of alvin000009238 March 31, 2026 06:24 View session

Copilot AI reviewed Mar 31, 2026

View reviewed changes

alvin000009238 added 3 commits March 31, 2026 14:41

Harden early theme init against storage access failures

d36cb9b

Merge branch 'dev' into codex/fix-csp-issue-with-inline-theme-init-sc…

1c75fec

…ript-i8s1np

Merge pull request #95 from alvin000009238/codex/fix-csp-issue-with-i…

8c9a35f

…nline-theme-init-script-i8s1np Cache chart theme palette, add theme-init, expose chart test hooks, and add theme tests

alvin000009238 merged commit bd825fb into main Mar 31, 2026
2 checks passed

Navigation Menu

Implement light/dark theme toggle with CSS tokens and chart theming - #91

Implement light/dark theme toggle with CSS tokens and chart theming#91
alvin000009238 merged 29 commits into
mainfrom
dev

alvin000009238 commented Mar 31, 2026

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

gemini-code-assist Bot Mar 31, 2026

Uh oh!

gemini-code-assist Bot Mar 31, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 31, 2026

Uh oh!

Copilot AI Mar 31, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 31, 2026

Uh oh!

Copilot AI Mar 31, 2026

Uh oh!

Copilot AI Mar 31, 2026

Uh oh!

Copilot AI Mar 31, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		grid: readVar('--color-border-subtle', 'rgba(148, 163, 184, 0.22)'),
		gridSubtle: readVar('--color-border-subtle', 'rgba(148, 163, 184, 0.16)')

Conversation

alvin000009238 commented Mar 31, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants