Conversation
dosubot
bot
added
the
size:L
![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 pull request implements a dark mode feature for the Trilium Web Clipper, updating both the options page and the popup with theme-specific CSS variables and toggle logic. Feedback focuses on refactoring duplicated code into a shared utility, adopting the browser.storage API instead of localStorage for better extension compatibility, and improving code maintainability by moving inline styles to CSS. Additionally, a transition effect was suggested for the popup to ensure a smooth visual experience during theme changes.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a dark mode feature for the Trilium Web Clipper, updating both the options page and the popup UI. The changes include the introduction of CSS variables for theme management, a toggle button, and persistent storage of the user's preference. The review feedback suggests awaiting the asynchronous theme loading in the popup to prevent visual flickering and recommends refactoring duplicated JavaScript logic and CSS definitions into shared files to improve maintainability.
| async function loadDarkModePreference() { | ||
| const {darkMode} = await browser.storage.sync.get<{ darkMode: boolean }>("darkMode"); | ||
| if (darkMode) { | ||
| document.body.classList.add('dark-mode'); | ||
| $darkModeButton.text('Light mode'); | ||
| } | ||
| } | ||
|
|
||
| $darkModeButton.on("click", async () => { | ||
| const isDarkMode = document.body.classList.toggle('dark-mode'); | ||
| await browser.storage.sync.set({darkMode: isDarkMode}); | ||
| $darkModeButton.text(isDarkMode ? 'Light mode' : 'Dark mode'); | ||
| }); |
There was a problem hiding this comment.
This dark mode logic (loading preference and handling the toggle) is identical to the implementation in apps/web-clipper/entrypoints/popup/popup.ts. To improve maintainability and prevent logic divergence, consider refactoring this into a shared utility function.
| :root { | ||
| --bg-color: #ffffff; | ||
| --text-color: #000000; | ||
| --button-bg: #eee; | ||
| --button-border: #ccc; | ||
| --textarea-bg: #ffffff; | ||
| --textarea-text: #000000; | ||
| } | ||
|
|
||
| body.dark-mode { | ||
| --bg-color: #1e1e1e; | ||
| --text-color: #e0e0e0; | ||
| --button-bg: #333333; | ||
| --button-border: #555555; | ||
| --textarea-bg: #2d2d2d; | ||
| --textarea-text: #e0e0e0; | ||
| } |
There was a problem hiding this comment.
The dark mode color palette and variable definitions are duplicated between popup.css and options.css. Refactoring these into a shared CSS file would ensure consistency and simplify future theme updates.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a dark mode feature for the Trilium Web Clipper, including theme persistence and UI toggles in both the options and popup pages. Feedback suggests extracting the duplicated dark mode logic into a shared utility and consolidating redundant CSS styles to improve maintainability. There is also a recommendation to use the internal sendMessage wrapper in popup.ts to ensure consistent error handling.
| async function loadDarkModePreference() { | ||
| const {darkMode} = await browser.storage.sync.get<{ darkMode: boolean }>("darkMode"); | ||
| if (darkMode) { | ||
| document.body.classList.add('dark-mode'); | ||
| $darkModeButton.text('Light mode'); | ||
| } else { | ||
| document.body.classList.remove('dark-mode'); | ||
| $darkModeButton.text('Dark mode'); | ||
| } | ||
| } | ||
|
|
||
| $darkModeButton.on("click", async () => { | ||
| const isDarkMode = document.body.classList.toggle('dark-mode'); | ||
| await browser.storage.sync.set({darkMode: isDarkMode}); | ||
| $darkModeButton.text(isDarkMode ? 'Light mode' : 'Dark mode'); | ||
| }); |
There was a problem hiding this comment.
The logic for managing dark mode (loading the preference and handling the toggle) is identical to the implementation in popup.ts. To improve maintainability and ensure consistent behavior across the extension, consider extracting this logic into a shared utility function in a common file (e.g., under utils/).
1 participant
Adds dark mode to the web clipper popup and options