added multiple variables instead of static/harcoded colors #724
Conversation
📝 WalkthroughWalkthroughThis PR refactors CSS color definitions across six stylesheet files to use CSS custom properties (variables) from openwisp-utils instead of hard-coded color values. The changes maintain all existing visual styling and DOM structure while consolidating color theme management to standardized CSS variable tokens. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Poem
Pre-merge checks✅ Passed checks (5 passed)
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
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
openwisp_monitoring/device/static/monitoring/css/daterangepicker.cssopenwisp_monitoring/device/static/monitoring/css/device-map.cssopenwisp_monitoring/device/static/monitoring/css/leaflet.fullscreen.cssopenwisp_monitoring/device/static/monitoring/css/monitoring.cssopenwisp_monitoring/device/static/monitoring/css/netjsongraph.cssopenwisp_monitoring/monitoring/static/monitoring/css/chart.css
🧰 Additional context used
🪛 Biome (2.1.2)
openwisp_monitoring/monitoring/static/monitoring/css/chart.css
[error] 114-114: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
font-size is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
[error] 124-124: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
font-size is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
⏰ 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.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | 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.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
🔇 Additional comments (7)
openwisp_monitoring/monitoring/static/monitoring/css/chart.css (1)
62-62: LGTM! Color variable replacements are correct.The hardcoded color values have been successfully replaced with CSS custom properties, maintaining the existing visual styling while enabling centralized theme management.
Also applies to: 67-67, 83-83, 113-113, 118-118, 126-126, 135-136
openwisp_monitoring/device/static/monitoring/css/daterangepicker.css (1)
4-4: LGTM! Comprehensive color variable replacement.All hardcoded color values have been successfully replaced with CSS custom properties across calendar elements, buttons, hover states, and active states. The changes maintain existing functionality while enabling centralized theme management.
Also applies to: 6-6, 23-23, 30-30, 35-35, 71-71, 77-77, 110-111, 141-141, 143-143, 153-153, 160-160, 166-168, 171-173, 187-189, 196-196, 221-222, 234-234, 241-241, 258-258, 261-261, 264-264, 267-267, 289-289, 292-293, 343-344, 347-348, 352-353, 364-366, 369-369, 372-372, 376-379, 381-381, 385-385, 391-392, 395-395
openwisp_monitoring/device/static/monitoring/css/leaflet.fullscreen.css (1)
2-2: LGTM! Color variable replacement and formatting improvement.The background color has been correctly updated to use the CSS variable, and the selector formatting improves readability without changing functionality.
Also applies to: 12-14
openwisp_monitoring/device/static/monitoring/css/device-map.css (1)
2-2: LGTM! Health status and paginator colors now use theme variables.The hardcoded colors for health states and pagination UI have been replaced with CSS custom properties, ensuring consistent theming across the application.
Also applies to: 5-5, 8-8, 14-14, 23-24, 29-30, 40-40
openwisp_monitoring/device/static/monitoring/css/monitoring.css (2)
5-7: LGTM! Color variables applied to device status styling.The hardcoded colors in headers, tables, and health status classes have been successfully replaced with CSS custom properties. The selector refactoring for health checks improves readability without changing functionality.
Also applies to: 34-34, 42-42, 45-45, 48-48, 54-54, 90-97
137-177: LGTM! Percircle theme overrides enhance consistency.The new Percircle Theme Overrides section applies CSS custom properties to the percircle component, including base styles, hover states, and color variants. This centralizes the theming for these chart elements.
Note: Orange colors remain hardcoded (lines 173, 176), which is consistent with the existing usage at line 51 for
.health-problem.openwisp_monitoring/device/static/monitoring/css/netjsongraph.css (1)
10-13: LGTM! Color variables with moderncolor-mixfunction.The hardcoded colors have been replaced with CSS custom properties throughout the netjsongraph styling. The use of
color-mix()for status indicator transparency is a modern CSS approach that maintains visual consistency while enabling theme flexibility.Also applies to: 47-48, 51-55, 61-65, 68-68, 84-84, 100-100, 107-107
| #ow-chart-contents .circle > span { | ||
| font-size: 0.15em; | ||
| word-spacing: -0.75px; | ||
| color: rgba(0, 0, 0, 0.6) !important; | ||
| color: var(--ow-overlay-60) !important; | ||
| font-size: 13px; | ||
| font-weight: bold; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove duplicate font-size declaration.
The font-size property is declared twice (lines 111 and 114), causing the second value to override the first. This is confusing and should be consolidated.
🔎 Proposed fix
#ow-chart-contents .circle > span {
- font-size: 0.15em;
word-spacing: -0.75px;
color: var(--ow-overlay-60) !important;
font-size: 13px;
font-weight: bold;
}Based on static analysis hints from Biome.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #ow-chart-contents .circle > span { | |
| font-size: 0.15em; | |
| word-spacing: -0.75px; | |
| color: rgba(0, 0, 0, 0.6) !important; | |
| color: var(--ow-overlay-60) !important; | |
| font-size: 13px; | |
| font-weight: bold; | |
| } | |
| #ow-chart-contents .circle > span { | |
| word-spacing: -0.75px; | |
| color: var(--ow-overlay-60) !important; | |
| font-size: 13px; | |
| font-weight: bold; | |
| } |
🧰 Tools
🪛 Biome (2.1.2)
[error] 114-114: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
font-size is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
🤖 Prompt for AI Agents
In openwisp_monitoring/monitoring/static/monitoring/css/chart.css around lines
110 to 116, there's a duplicate font-size declaration in the selector
#ow-chart-contents .circle > span (font-size: 0.15em and font-size: 13px);
remove the redundant declaration and keep the intended value (convert to the
consistent unit if needed), leaving a single font-size line so the rule is clear
and not overridden.
| .chart-heading { | ||
| margin: 0; | ||
| font-size: 2em; | ||
| text-align: center; | ||
| font-size: 1.22em; | ||
| font-weight: normal; | ||
| color: #434b62; | ||
| color: var(--ow-color-fg-darker); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove duplicate .chart-heading rule.
This .chart-heading selector block (lines 120-127) is a complete duplicate of the rule at lines 56-63. The duplicate should be removed to avoid confusion and potential maintenance issues.
🔎 Proposed fix
#ow-chart-contents b {
background-color: var(--ow-color-black) !important;
}
-.chart-heading {
- margin: 0;
- font-size: 2em;
- text-align: center;
- font-size: 1.22em;
- font-weight: normal;
- color: var(--ow-color-fg-darker);
-}
#chart-loading-overlay {
width: 100%;
margin: 25px auto;
display: none;
}Based on static analysis hints from Biome.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .chart-heading { | |
| margin: 0; | |
| font-size: 2em; | |
| text-align: center; | |
| font-size: 1.22em; | |
| font-weight: normal; | |
| color: #434b62; | |
| color: var(--ow-color-fg-darker); | |
| } | |
| #ow-chart-contents b { | |
| background-color: var(--ow-color-black) !important; | |
| } | |
| #chart-loading-overlay { | |
| width: 100%; | |
| margin: 25px auto; | |
| display: none; | |
| } |
🧰 Tools
🪛 Biome (2.1.2)
[error] 124-124: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
font-size is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
🤖 Prompt for AI Agents
In openwisp_monitoring/monitoring/static/monitoring/css/chart.css around lines
120-127, there is a duplicate .chart-heading rule that repeats the same
declarations already defined at lines 56-63; remove the redundant block (lines
120-127) so only the original .chart-heading rule remains to avoid duplication
and maintenance confusion.
1 participant
Checklist
Reference to Existing Issue
Closes #720
Description of Changes
replace all the hardcoded colors from variables like openwisp/openwisp-utils#516.
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.