CSS cleanup and light mode - #70
Conversation
Implements a theme system for the settings and popup UI with three modes: - Light: Force light mode - Auto: Follow system preferences (default) - Dark: Force dark mode Changes: - Added theme.ts: Core theme detection and application logic - Updated types.ts: Added theme field to StorageData - Updated settings.css & popup.css: Added data-theme attribute support - Updated settings.html: Added Appearance section with theme selector - Updated settings.ts: Integrated theme initialization and event handlers - Updated popup.ts: Added theme initialization - Updated browser-mock.ts: Added theme to mock storage The UI now respects user theme preference while maintaining backward compatibility with system theme detection via media queries.
- Consolidate all CSS into src/styles/ for better organization - Update HTML file references to new paths - Update vite.config.ts build copy logic - Add CLAUDE.md with CSS best practices and known issues
- Add z-index scale and transition duration tokens - Add toggle switch semantic tokens for theming - Fix hardcoded colors in phrase-tag and toggle-slider - Add prefers-reduced-motion support for accessibility - Remove deprecated -webkit-overflow-scrolling vendor prefix - Update z-index values to use design tokens - Add explanatory comment for dark mode duplication
|
🤖 Hi @gerrywastaken, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
|
ext: makeitpop-build-2ba77eb.zip |
Summary of ChangesHello @gerrywastaken, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the extension's styling infrastructure by introducing light mode support and performing a major CSS cleanup. The changes aim to improve maintainability, consistency, and user experience by adopting a robust design token system, reorganizing stylesheets for better modularity, and providing users with theme customization options. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is an excellent and substantial pull request that significantly improves the project's CSS architecture and adds a valuable theme-switching feature. The move to a token-based design system in shared.css is a fantastic step for maintainability, and the new CLAUDE.md style guide will help keep the CSS consistent going forward. The light/dark mode implementation is well-thought-out, correctly handling both user preference and system settings.
I've found a couple of issues, one high-severity issue related to a redundant initialization call in the new theme logic, which could lead to duplicate event listeners, and a medium-severity suggestion to improve how theme changes are reflected in the UI. My review comments include fixes and suggestions.
Overall, this is a high-quality contribution that modernizes the frontend codebase. Great work!
| // Initialize theme system | ||
| async function initThemeUI() { | ||
| // Initialize theme engine | ||
| await initTheme(); |
There was a problem hiding this comment.
This call to initTheme() is redundant. The theme is already initialized in the main IIFE at the bottom of the file (line 660). Calling initTheme() twice will attach duplicate event listeners for system theme changes and storage changes, which can lead to unnecessary processing and potential bugs. Please remove this line.
| export function updatePreviewDefaults(theme: 'light' | 'dark') { | ||
| currentEffectiveTheme = theme; | ||
|
|
||
| // Find all group cards and update their preview toggle to match the new theme | ||
| const cards = document.querySelectorAll('#groupsList .card'); | ||
| cards.forEach(card => { | ||
| const lightBtn = card.querySelector('.mode-toggle-btn:first-child') as HTMLElement; | ||
| const darkBtn = card.querySelector('.mode-toggle-btn:last-child') as HTMLElement; | ||
| if (lightBtn && darkBtn) { | ||
| // Update button states | ||
| lightBtn.classList.toggle('active', theme === 'light'); | ||
| darkBtn.classList.toggle('active', theme === 'dark'); | ||
|
|
||
| // Switch preview visibility | ||
| const previews = card.querySelectorAll('.mode-preview'); | ||
| previews.forEach(p => { | ||
| const preview = p as HTMLElement; | ||
| const previewMode = preview.getAttribute('data-mode'); | ||
| preview.classList.toggle('active', previewMode === theme); | ||
| }); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
The updatePreviewDefaults function manually queries and iterates over all group cards in the DOM to update their styles when the theme changes. A more performant and idiomatic CSS approach would be to leverage the data-theme attribute on the <html> element, which is already being set by your new theme logic. This would eliminate the need for this JavaScript-based DOM manipulation.
For example, you could replace the logic here with CSS rules in settings.css:
/* Show the correct preview based on the theme */
.mode-preview {
display: none;
}
[data-theme='light'] .mode-preview[data-mode='light'],
[data-theme='dark'] .mode-preview[data-mode='dark'] {
display: block;
}
/* Style the active toggle button */
[data-theme='light'] .mode-toggle-btn:first-child,
[data-theme='dark'] .mode-toggle-btn:last-child {
/* ... active styles ... */
background: var(--card-bg);
color: var(--text-color);
}This would make the component more declarative and less reliant on imperative JavaScript for styling.
There was a problem hiding this comment.
Invalid, the previews can be set independent of the user's current theme.
You misunderstood the feature. The light/dark preview toggle in group cards is for previewing highlight colors on target websites - it's independent of the settings UI theme. A user in dark mode UI might want to preview how their highlights look on a light-mode website. Tying preview visibility to [data-theme] would break this functionality.
|
🤖 I'm sorry @gerrywastaken, but I was unable to process your request. Please see the logs for more details. |
initTheme() is already called in the main IIFE before initThemeUI(), so calling it again inside initThemeUI() attached duplicate event listeners for system theme and storage changes.
Use --transition-normal (0.2s) and --transition-slow (0.3s) tokens throughout shared.css and settings.css for consistency.
|
@codex is there anything user facing and relevant that should be added to the changelog. If so please create a pr |
|
Summary
Testing
|
2 participants
This PR does too much... I know, but it is worth it and not worth the PR refactor
It gives us light mode support and does a massive CSS cleanup to follow better design and engineering practices to make maintenance easier.
It Is Essentially 3 PRs, but at least They Are 3 separate Commits.
Fix CSS issues: tokens, reduced-motion, hardcoded values
Move CSS files to styles directory with guidelines
Add light mode support with auto/manual theme switching
Implements a theme system for the settings and popup UI with three modes:
Changes:
The UI now respects user theme preference while maintaining backward
compatibility with system theme detection via media queries.