[chores] Replace hardcoded colors with CSS variables and format CSS files#1164
[chores] Replace hardcoded colors with CSS variables and format CSS files#1164atif09 wants to merge 9 commits intoopenwisp:masterfrom
Conversation
atif09
changed the title
…#442 Problem: Some tests in TestConfigApi assumed a fixed global Template count; which fail when other modules like openwisp-monitoring add new templates. Fix: Updated template-related assertions in test_api.py to compare counts relatively (eg: capture initial_count, assert +1). Fixes openwisp#442 * [tests] Remove unnecessary test settings file openwisp#442 The test_settings override is not required to fix the template counting issue and adds unrelated changes. Removing it to keep the fix minimal and focused. Fixes openwisp#442
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request replaces hard-coded color values with CSS custom properties across multiple CSS files in openwisp-controller (admin.css, advanced-mode.css, jsonschema-ui.css, import-openwisp.css, command-inline.css, subnet-division.css). Affected selectors include form controls, JSON editor UI, overlays, dialogs, buttons, tables, preformatted text, and other UI components, switching explicit hex/RGBA values to theme variables (e.g., var(--ow-...)). No functional logic, control flow, or public API declarations were modified. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_controller/config/static/config/css/lib/jsonschema-ui.css (1)
383-391: Missed hardcoded color value.Line 386 still contains
background-color: white;which should be replaced withvar(--ow-color-white)to align with the PR objective of eliminating hardcoded colors.Proposed fix
.jsoneditor-wrapper div.jsoneditor .modal { position: absolute; z-index: 10; - background-color: white; + background-color: var(--ow-color-white); border: 1px solid var(--ow-color-fg-light); padding-bottom: 10px; width: 340px; margin-left: -215px; }
🤖 Fix all issues with AI agents
In `@openwisp_controller/config/static/config/css/admin.css`:
- Around line 119-127: The hover rule for .djnjc-overlay .close currently sets
the same background as the base .djnjc-overlay .close so there is no visible
change; update the hover to produce a visible effect by either (a) using an
existing token like var(--ow-color-primary-light) (plus a contrasting color
change such as color or border) or (b) applying a visual tweak such as reduced
opacity (e.g. background-color with rgba/opacity) or a slight
transform/box-shadow, or (c) add and define a new token (e.g.
--ow-color-primary-dark) in your theme if you need a darker variant; modify the
.djnjc-overlay .close:hover selector accordingly and ensure contrast remains
accessible.
In `@openwisp_controller/connection/static/connection/css/command-inline.css`:
- Around line 121-134: The hover/focus rules for `#ow-command-confirm-yes` and
`#ow-command-confirm-no` currently reuse the same background variables and provide
no visual change; replace the nonexistent hover vars with existing tokens or
other visual feedback: update the hover/focus selectors for
`#ow-command-confirm-yes` to use a darker/contrasting variable (e.g.
var(--ow-color-fg-dark) or a semi-transparent overlay) or add a subtle
outline/box-shadow and do the same for `#ow-command-confirm-no` (use a darker
danger/contrast token or opacity change) so users get clear hover/focus feedback
without introducing undefined variables.
🧹 Nitpick comments (4)
openwisp_controller/config/static/config/css/lib/advanced-mode.css (4)
112-114: Semantic concern: Using danger color for number values.Similar to the URL hover issue,
--ow-btn-danger-bgis semantically meant for destructive actions. While JSON editors often use reddish colors for numbers, reusing the "danger" variable could cause confusion in theming. If a dedicated--ow-color-numberor similar doesn't exist, consider documenting this choice or using a more neutral variable.
426-431: Consider using an error/warning color for error display.The
.jsoneditor-text-errorselement displays validation errors, but uses--ow-color-fg-lighter(neutral) instead of an error-related variable. While this provides a subtle appearance, using--ow-color-error-bgor similar (if available) would better communicate the error state semantically.
612-617: Semantic concern: Using danger color for selected state.
--ow-btn-danger-bgfor.jsoneditor-selectedbutton background may confuse users, as red typically signals danger. Consider using--ow-color-primaryor a dedicated selection color variable for better semantic clarity.
75-78: Semantic mismatch: Using danger color for link hover state
--ow-btn-danger-bgis reserved for destructive actions (delete buttons, error states, validation errors). Using it for URL hover/focus states creates confusing semantics—users may interpret the red color as an error or warning. URL hover states should use a neutral or primary color variable instead to maintain proper visual hierarchy and UX clarity.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
openwisp_controller/config/static/config/css/admin.cssopenwisp_controller/config/static/config/css/lib/advanced-mode.cssopenwisp_controller/config/static/config/css/lib/jsonschema-ui.cssopenwisp_controller/config/static/import_export/import-openwisp.cssopenwisp_controller/connection/static/connection/css/command-inline.cssopenwisp_controller/subnet_division/static/subnet-division/css/subnet-division.css
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
🔇 Additional comments (22)
openwisp_controller/config/static/config/css/lib/jsonschema-ui.css (2)
15-25: LGTM!Border and button background colors are appropriately replaced with semantic CSS variables (
--ow-color-fg-light,--ow-btn-danger-bg).
491-498: LGTM!The inline-related header styling correctly uses semantic variables for text color, background, and borders, providing consistent theming.
openwisp_controller/subnet_division/static/subnet-division/css/subnet-division.css (2)
1-4: LGTM!The readonly input styling correctly uses overlay variables for border and background, replacing the previous rgba values.
5-11: Verify if openwisp-utils provides warning-specific color variables.The class
.help-text-warninguses--ow-color-primary-lightfor its background. While using a primary brand color for warning context may lack semantic clarity, this concern depends on whether openwisp-utils (v1.3) provides dedicated warning or alert color variables. No warning-specific variables are used anywhere in openwisp-controller, so verify with the openwisp-utils documentation whether--ow-color-warning,--ow-color-alert, or similar variables exist and should be used here instead.openwisp_controller/config/static/config/css/admin.css (3)
5-23: LGTM!The preformatted and JSON editor text styling correctly uses semantic variables for foreground/background colors, maintaining the monospace code appearance.
30-33: LGTM!Readonly input styling matches the same pattern used in
subnet-division.css, ensuring consistency across the codebase.
249-279: LGTM!Tab styling correctly uses semantic variables for different states (default, hover, current), providing consistent theming for navigation elements.
openwisp_controller/connection/static/connection/css/command-inline.css (2)
41-52: LGTM!The command overlay and dialog styling appropriately uses semantic variables for backdrop, border, and shadow colors.
101-114: LGTM!The confirmation dialog styling correctly uses semantic variables for background and text colors, with appropriate font-family declaration.
openwisp_controller/config/static/import_export/import-openwisp.css (1)
15-38: The implementation is correct. Using CSS variables within a@media (prefers-color-scheme: dark)block is the recommended pattern for openwisp-utils, as the variables themselves are static light-mode values and are designed to be overridden in prefers-color-scheme media queries. The code correctly forces consistent light-theme colors when the browser requests dark mode, which aligns with the stated intent that dark theme is not yet fully supported in OpenWISP.openwisp_controller/config/static/config/css/lib/advanced-mode.css (12)
84-103: LGTM!Consistent use of
--ow-color-fg-lighterfor highlight and focus states provides visual coherence.
155-162: LGTM!Appropriate use of neutral foreground variables for button focus states.
167-179: LGTM!Good semantic usage—primary color for the container border establishes visual hierarchy, and darker foreground for text ensures readability.
289-299: LGTM!Appropriate variable choices for the popover: dark background with white text ensures good contrast, and overlay variable for shadows is semantically correct.
325-355: LGTM!Arrow border colors correctly match the popover background for visual consistency.
461-472: LGTM!Appropriate use of neutral border and overlay variables for the context menu.
704-714: LGTM!Primary color for the menu bar is semantically appropriate and establishes clear visual hierarchy.
730-740: LGTM!Consistent use of light overlay for interactive states on the dark menu background.
787-803: LGTM!Good contrast choices for the exit button with appropriate hover state feedback.
898-908: LGTM!White background for full-screen mode is appropriate.
915-924: LGTM!White text for menu labels maintains proper contrast against the primary-colored menu bar.
531-542: Formatting changes look acceptable.These multi-line selector reformats appear to be Prettier output. While the line-per-selector style increases line count, it improves readability for complex selectors.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
…1162 Replaced all hardcoded color literals (gray, lightgray, green, yellow, white) in advanced-mode.css and jsonschema-ui.css with semantic CSS variables from openwisp-utils for consistent theming support. Changes: - advanced-mode.css: Replaced 14 hardcoded colors with appropriate CSS variables (--ow-color-fg-medium, --ow-color-fg-light, --ow-color-success, --ow-color-warning, --ow-color-white) - jsonschema-ui.css: Replaced 1 hardcoded white with --ow-color-white This completes the color variable migration started in the initial commit. Fixes openwisp#1162
Replaced hardcoded color values with OpenWISP CSS variables to support theming and maintain consistency with the design system. Fixes openwisp#1162
|
nemesifier
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_controller/connection/static/connection/css/command-inline.css`:
- Around line 71-73: The .ow-error-input rule uses the button background
variable --ow-btn-danger-bg for its border which is semantically mismatched; if
the shared design tokens include a generic danger colour (e.g.
--ow-color-danger) in openwisp-utils, replace --ow-btn-danger-bg with that
variable in the .ow-error-input declaration to express intent clearly; if no
such generic variable exists, leave the current value but add a short comment
above .ow-error-input noting the intentional reuse of --ow-btn-danger-bg for
consistency.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
openwisp_controller/connection/static/connection/css/command-inline.css
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: atif09
Repo: openwisp/openwisp-controller PR: 1164
File: openwisp_controller/connection/static/connection/css/command-inline.css:121-134
Timestamp: 2026-01-16T10:20:30.847Z
Learning: In openwisp-controller, when replacing hardcoded colors with CSS variables, it's acceptable to use the same variable for both default and hover/focus states even if the original code had different colors for these states. UX improvements like restoring hover state visual feedback are considered out of scope for color replacement PRs and can be addressed separately.
📚 Learning: 2026-01-16T10:20:24.194Z
Learnt from: atif09
Repo: openwisp/openwisp-controller PR: 1164
File: openwisp_controller/connection/static/connection/css/command-inline.css:121-134
Timestamp: 2026-01-16T10:20:24.194Z
Learning: In CSS files under openwisp_controller/connection/static/connection/css, when replacing hardcoded colors with CSS variables, you may reuse the same CSS variable for default and hover/focus states even if the original code used different colors. Note that UX improvements related to hover state visual feedback are out of scope for color-replacement PRs and can be addressed separately. Ensure the change remains focused on color substitution and does not introduce behavior changes beyond color values.
Applied to files:
openwisp_controller/connection/static/connection/css/command-inline.css
🔇 Additional comments (5)
openwisp_controller/connection/static/connection/css/command-inline.css (5)
41-43: LGTM!Appropriate replacement of
rgba(0,0,0,0.6)with the semantic overlay variable.
44-52: LGTM!Border and box-shadow colors appropriately use semantic theme variables.
101-105: LGTM!Dialog background appropriately uses the white color variable.
106-114: LGTM!Text color appropriately uses the dark foreground variable.
121-134: LGTM!Button color variables are correctly applied. The use of the same variable for default and hover/focus states is acceptable per the PR scope (color replacement only). Based on learnings, UX improvements for hover state visual feedback are out of scope for this PR and can be addressed separately.
| .ow-error-input { | ||
| border: 1px #ba2121 solid; | ||
| border: 1px var(--ow-btn-danger-bg) solid; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Acceptable, though note the semantic mismatch.
Using --ow-btn-danger-bg for an input border works for color consistency, but semantically this variable is intended for button backgrounds. If a more generic danger color variable (e.g., --ow-color-danger) exists in openwisp-utils, consider using it here for clearer intent. Otherwise, this is fine for the scope of this PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_controller/connection/static/connection/css/command-inline.css`
around lines 71 - 73, The .ow-error-input rule uses the button background
variable --ow-btn-danger-bg for its border which is semantically mismatched; if
the shared design tokens include a generic danger colour (e.g.
--ow-color-danger) in openwisp-utils, replace --ow-btn-danger-bg with that
variable in the .ow-error-input declaration to express intent clearly; if no
such generic variable exists, leave the current value but add a short comment
above .ow-error-input noting the intentional reuse of --ow-btn-danger-bg for
consistency.
4 participants
Checklist
Reference to Existing Issue
Closes #1162 .
Description of Changes
Replaces hardcoded color literals (hex & rgba) in the UI CSS with the project's semantic CSS variables defined in openwisp-utils#516
Screenshot
N/A