Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements a standalone dark mode feature for the application, providing users with the ability to toggle between light and dark themes. The implementation is designed to be non-invasive, using CSS data attributes and vanilla JavaScript to avoid interference with existing React components.
Key Changes:
- Created independent dark mode CSS styling system using
[data-theme="dark"]selectors - Implemented vanilla JavaScript toggle mechanism with localStorage persistence and OS preference detection
- Added minimalist moon/sun icon toggle button in the navbar using Bootstrap Icons
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| src/styles/dark-mode.css | Comprehensive dark mode styles for all UI components using CSS data attribute selectors |
| src/scripts/darkModeToggle.js | Vanilla JavaScript implementation for theme toggling, persistence, and button management |
| src/main.jsx | Added import for dark mode toggle script |
| src/index.css | Added import for dark mode CSS stylesheet |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [data-theme="dark"] b { | ||
| background: var(--black-500) !important; | ||
| color: var(--base-white) !important; | ||
| } | ||
|
|
||
| [data-theme="dark"] abbr { | ||
| background: var(--black-500) !important; | ||
| color: var(--base-white) !important; | ||
| } |
There was a problem hiding this comment.
Applying dark mode styles to all <b> and <abbr> elements globally may have unintended side effects. These are semantic HTML elements that might be used in various contexts throughout the application. Consider using more specific selectors (e.g., scoping to specific components or adding a class) to avoid affecting elements that shouldn't have this styling.
| [data-theme="dark"] b { | |
| background: var(--black-500) !important; | |
| color: var(--base-white) !important; | |
| } | |
| [data-theme="dark"] abbr { | |
| background: var(--black-500) !important; | |
| color: var(--base-white) !important; | |
| } | |
| [data-theme="dark"] .homepage-highlight { | |
| background: var(--black-500) !important; | |
| color: var(--base-white) !important; | |
| } |
| /* Variáveis auxiliares (não utilizadas atualmente, podem ser removidas) */ | ||
| [data-theme="dark"] { | ||
| --bg-main: var(--base-black); | ||
| --bg-card: var(--black-700); | ||
| --bg-hover: var(--black-500); | ||
| --txt-primary: var(--base-white); | ||
| --txt-secondary: var(--white-500); | ||
| --txt-tertiary: var(--white-700); | ||
| } | ||
|
|
There was a problem hiding this comment.
The comment on line 7 states these CSS variables are "não utilizadas atualmente, podem ser removidas" (not currently used, can be removed). These unused CSS custom properties add unnecessary code. Consider removing them to keep the codebase clean and maintainable.
| /* Variáveis auxiliares (não utilizadas atualmente, podem ser removidas) */ | |
| [data-theme="dark"] { | |
| --bg-main: var(--base-black); | |
| --bg-card: var(--black-700); | |
| --bg-hover: var(--black-500); | |
| --txt-primary: var(--base-white); | |
| --txt-secondary: var(--white-500); | |
| --txt-tertiary: var(--white-700); | |
| } |
| [data-theme="dark"] * { | ||
| transition: background-color 0.3s ease, color 0.3s ease, border-color 0.3s ease; | ||
| } |
There was a problem hiding this comment.
Applying a transition to all elements with the universal selector (*) can cause significant performance issues, especially on pages with many DOM elements. Each property change will trigger transitions on every element, which can lead to janky animations and increased CPU usage. Consider applying transitions only to specific elements that need them, or at minimum scope this to only the properties that actually change (background-color, color, border-color) on specific selectors rather than using the universal selector.
| [data-theme="dark"] * { | |
| transition: background-color 0.3s ease, color 0.3s ease, border-color 0.3s ease; | |
| } | |
| [data-theme="dark"] body, | |
| [data-theme="dark"] div#root { | |
| transition: background-color 0.3s ease, color 0.3s ease; | |
| } | |
| [data-theme="dark"] b, | |
| [data-theme="dark"] abbr { | |
| transition: background-color 0.3s ease, color 0.3s ease; | |
| } | |
| [data-theme="dark"] button[class*="outlined"], | |
| [data-theme="dark"] a[class*="outlined"] { | |
| transition: border-color 0.3s ease, color 0.3s ease, background-color 0.3s ease; | |
| } | |
| [data-theme="dark"] button[class*="outlined"]:hover, | |
| [data-theme="dark"] a[class*="outlined"]:hover { | |
| transition: background-color 0.3s ease, border-color 0.3s ease, color 0.3s ease; | |
| } |
| updateToggleButton(initialTheme); | ||
| } else if (attempts < 20) { | ||
| // Tenta novamente após 100ms | ||
| setTimeout(() => tryCreateButton(attempts + 1), 100); |
There was a problem hiding this comment.
The tryCreateButton function will attempt to find the navbar up to 20 times with 100ms intervals (total 2 seconds). However, if the navbar structure changes or the selector .navbar-nav:last-child doesn't match the actual DOM structure, this will silently fail after all attempts. Consider adding a console warning or error after all attempts are exhausted to help with debugging, or verify that this selector accurately matches your navbar structure.
| setTimeout(() => tryCreateButton(attempts + 1), 100); | |
| setTimeout(() => tryCreateButton(attempts + 1), 100); | |
| } else { | |
| console.warn( | |
| "[DarkModeToggle] Failed to find navbar with selector '.navbar-nav:last-child' after 20 attempts. " + | |
| "Button not created. Please check if the selector matches your DOM structure." | |
| ); |
| // Expõe funções globalmente (opcional, para debug) | ||
| window.darkMode = { | ||
| toggle: toggleTheme, | ||
| set: applyTheme, | ||
| get: () => document.documentElement.getAttribute('data-theme') | ||
| }; |
There was a problem hiding this comment.
[nitpick] The window.darkMode object is exposed globally for debugging purposes, but this could lead to unintended side effects if other code modifies these functions or if there are naming conflicts. Consider either removing this exposure in production code, or namespacing it more specifically (e.g., window.__DESCRIPTUM_DARK_MODE__) to avoid potential conflicts.
| const button = document.createElement('button'); | ||
| button.id = 'dark-mode-toggle'; | ||
| button.setAttribute('aria-label', 'Alternar modo escuro'); | ||
| button.innerHTML = '<i class="bi bi-moon-stars-fill"></i>'; |
There was a problem hiding this comment.
The button has an aria-label attribute but it's static ("Alternar modo escuro"). Consider using aria-pressed attribute to indicate the current state of the toggle button for better accessibility. The button should have aria-pressed="false" in light mode and aria-pressed="true" in dark mode to communicate the toggle state to screen readers.
| const title = theme === 'light' ? 'Ativar modo escuro' : 'Ativar modo claro'; | ||
| button.innerHTML = icon; | ||
| button.setAttribute('title', title); |
There was a problem hiding this comment.
The updateToggleButton function updates the title attribute but should also update the aria-label to match the current state. Additionally, consider adding aria-pressed attribute here to indicate whether dark mode is currently active (true) or inactive (false) for better accessibility support.
| const title = theme === 'light' ? 'Ativar modo escuro' : 'Ativar modo claro'; | |
| button.innerHTML = icon; | |
| button.setAttribute('title', title); | |
| const title = theme === 'light' ? 'Ativar modo escuro' : 'Ativar modo claro'; | |
| const ariaLabel = title; // Use the same string for aria-label | |
| const ariaPressed = theme === 'dark' ? 'true' : 'false'; | |
| button.innerHTML = icon; | |
| button.setAttribute('title', title); | |
| button.setAttribute('aria-label', ariaLabel); | |
| button.setAttribute('aria-pressed', ariaPressed); |
|
Este PR foi integrado ao PR #20 (feat/user-guide-page) durante o desenvolvimento. As funcionalidades de dark mode implementadas aqui foram:
Os commits relevantes são:
Ação: Fechando este PR em favor do #20 que contém todo o código atualizado. |
Dark Mode - Implementação Independente
Tipo de Pull Request:
Referência da Task
Jira/Ticket: CDB-113
Descrição das Alterações - Changelog
Instruções de Teste
Passos para testar:
git pullenpm install(não há novas dependências, apenas garantia)npm run devResultado esperado:
Reviewers Recomendados
Observações
Arquivos Criados:
src/styles/dark-mode.css- Estilos do modo escurosrc/scripts/darkModeToggle.js- Lógica de gerenciamento do temaArquivos Modificados (apenas imports):
src/index.css- Adicionada 1 linha de importsrc/main.jsx- Adicionada 1 linha de importCaracterísticas Técnicas:
[data-theme="dark"]: Só aplica quando ativo!importantpara sobrescrever sem modificar estilos originaisFuncionalidades Implementadas:
prefers-color-scheme)localStorageCompatibilidade: