[chores] Use theme colors from openwisp-utils variables #425#477
[chores] Use theme colors from openwisp-utils variables #425#477BHARATH0153 wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughThis PR replaces hard-coded color literals with OpenWISP CSS theme variables across notification-related stylesheets (loader.css, notifications.css, object-notifications.css, preferences.css), updating gradients, pseudo-element backgrounds, toast variants, dropdowns, dialog overlays, metadata/links, tooltips, and switch controls to use --ow-color-* and --ow-overlay-* variables. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
BHARATH0153
force-pushed
the
chores/issue-425-theme-colors
branch
from
f8bc219 to
0fb5741
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
openwisp_notifications/static/openwisp-notifications/css/notifications.css (1)
178-225: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider using a theme variable for text stroke color.
The color migrations look good overall. However, line 193 contains
-webkit-text-stroke: 0.2px white;which uses a hardcoded color value. Consider replacingwhitewithvar(--ow-color-white)for consistency with the rest of the migration.♻️ Proposed fix
display: flex; justify-content: space-between; color: var(--ow-color-fg-dark); - -webkit-text-stroke: 0.2px white; + -webkit-text-stroke: 0.2px var(--ow-color-white); padding: 1px 0px; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openwisp_notifications/static/openwisp-notifications/css/notifications.css` around lines 178 - 225, Replace the hardcoded color in the -webkit-text-stroke rule within the .ow-notification-meta selector by using the theme variable; update the declaration "-webkit-text-stroke: 0.2px white;" to use "var(--ow-color-white)" instead (i.e. "-webkit-text-stroke: 0.2px var(--ow-color-white);") so the .ow-notification-meta styling follows the theme variable convention.openwisp_notifications/static/openwisp-notifications/css/preferences.css (2)
361-364:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHard-coded colors remain in switch checked state.
Lines 361 and 364 contain
#2196f3(blue), which contradicts the PR objective to replace all hard-coded theme colors with openwisp-utils variables. These should be replaced with an appropriate theme variable, such asvar(--ow-color-primary).🎨 Proposed fix to use theme variable
input:checked + .slider { - background-color: `#2196f3`; + background-color: var(--ow-color-primary); } input:focus + .slider { - box-shadow: 0 0 5px 2px `#2196f3`; + box-shadow: 0 0 5px 2px var(--ow-color-primary); outline: none; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openwisp_notifications/static/openwisp-notifications/css/preferences.css` around lines 361 - 364, The CSS uses hard-coded blue (`#2196f3`) in the switch checked/focus styles; replace those occurrences with the theme variable (e.g., var(--ow-color-primary)) so both the background-color declaration and the box-shadow in the input:focus + .slider selector use the theme token instead of the literal color; update the rules that set background-color: `#2196f3` and box-shadow: 0 0 5px 2px `#2196f3` to use var(--ow-color-primary) (or the appropriate openwisp-utils variable) to ensure consistent theming.
258-258:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHard-coded color remains in progress bar.
Line 258 contains
#007bff(blue), which contradicts the PR objective to replace all hard-coded theme colors with openwisp-utils variables. This should be replaced with an appropriate theme variable, such asvar(--ow-color-primary)or a similar blue variable from the theme palette.🎨 Proposed fix to use theme variable
.toast .progress-bar { position: absolute; bottom: 0; left: 0; height: 4px; - background-color: `#007bff`; + background-color: var(--ow-color-primary); width: 100%; transition: width 3s linear; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openwisp_notifications/static/openwisp-notifications/css/preferences.css` at line 258, The progress bar CSS in preferences.css still uses a hard-coded color "background-color: `#007bff`"; replace that hard-coded value with the theme variable (e.g., var(--ow-color-primary) or the appropriate blue from the OpenWISP theme palette) so the rule uses the theme token instead of the literal hex value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@openwisp_notifications/static/openwisp-notifications/css/notifications.css`:
- Around line 178-225: Replace the hardcoded color in the -webkit-text-stroke
rule within the .ow-notification-meta selector by using the theme variable;
update the declaration "-webkit-text-stroke: 0.2px white;" to use
"var(--ow-color-white)" instead (i.e. "-webkit-text-stroke: 0.2px
var(--ow-color-white);") so the .ow-notification-meta styling follows the theme
variable convention.
In `@openwisp_notifications/static/openwisp-notifications/css/preferences.css`:
- Around line 361-364: The CSS uses hard-coded blue (`#2196f3`) in the switch
checked/focus styles; replace those occurrences with the theme variable (e.g.,
var(--ow-color-primary)) so both the background-color declaration and the
box-shadow in the input:focus + .slider selector use the theme token instead of
the literal color; update the rules that set background-color: `#2196f3` and
box-shadow: 0 0 5px 2px `#2196f3` to use var(--ow-color-primary) (or the
appropriate openwisp-utils variable) to ensure consistent theming.
- Line 258: The progress bar CSS in preferences.css still uses a hard-coded
color "background-color: `#007bff`"; replace that hard-coded value with the theme
variable (e.g., var(--ow-color-primary) or the appropriate blue from the
OpenWISP theme palette) so the rule uses the theme token instead of the literal
hex value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 41efb5c2-6127-4cdd-83b7-70836fce7586
📒 Files selected for processing (4)
openwisp_notifications/static/openwisp-notifications/css/loader.cssopenwisp_notifications/static/openwisp-notifications/css/notifications.cssopenwisp_notifications/static/openwisp-notifications/css/object-notifications.cssopenwisp_notifications/static/openwisp-notifications/css/preferences.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). (13)
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.12 | 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.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
🔇 Additional comments (12)
openwisp_notifications/static/openwisp-notifications/css/loader.css (1)
9-22: LGTM!Also applies to: 33-33, 41-41
openwisp_notifications/static/openwisp-notifications/css/object-notifications.css (1)
27-27: LGTM!Also applies to: 51-51
openwisp_notifications/static/openwisp-notifications/css/notifications.css (6)
71-102: LGTM!
141-167: LGTM!
230-236: LGTM!
302-364: LGTM!
1-391: ⚡ Quick winConfirm theme variable definitions in openwisp-utils (PR
#516)
The CSS variables referenced inopenwisp_notifications/static/openwisp-notifications/css/notifications.cssaren’t defined anywhere in this repository’s static CSS, so the mapping toopenwisp-utilsneeds to be checked there (with matching names) for:
- Color:
--ow-color-fg-dark,--ow-color-fg-darker,--ow-color-fg-light,--ow-color-primary,--ow-color-primary-light,--ow-color-white,--ow-color-black,--ow-color-danger,--ow-color-success,--ow-color-login-bg- Overlay:
--ow-overlay-7,--ow-overlay-10,--ow-overlay-25,--ow-overlay-40,--ow-overlay-60,--ow-overlay-90
25-46: ⚡ Quick winReplace hardcoded
#ffffff82box-shadow in notification badge (line 45) with an OpenWISP theme variable if one exists
- In
openwisp_notifications/static/openwisp-notifications/css/notifications.css, the bell/count badge still usesbox-shadow: 0px 0px 3px#ffffff82inset;while the surrounding colors were migrated to--ow-*variables.- This repository contains usages of
--ow-*(e.g.,--ow-color-white,--ow-overlay-*) but no theme definition/mapping for a “semi-transparent white” shadow token, so verify inopenwisp-utilsfor an equivalent shadow/overlay variable (or keep the hardcoded value if none exists).openwisp_notifications/static/openwisp-notifications/css/preferences.css (4)
37-79: LGTM!
138-141: LGTM!
238-239: LGTM!Also applies to: 314-315, 356-356
230-230: ⚡ Quick winConfirm/correct theme variable names for
--body-quiet-colorand--border-color
openwisp_notifications/static/openwisp-notifications/css/preferences.cssusesvar(--body-quiet-color)(e.g., lines 216/230/298/304) andvar(--border-color)(line 345), but neither variable is defined anywhere in this repo’s CSS, and the openwisp-utils sources found via search don’t show these exact variable names either. Replace them with the intended openwisp-utils--ow-*equivalents (or ensure these variables are actually provided by openwisp-utils in the shipped theme CSS).
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_notifications/static/openwisp-notifications/css/preferences.css (1)
345-356:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSwitch state colors still hardcoded after variable migration.
The base slider/knob now uses variables, but Line 361 and Line 364 still hardcode
#2196f3. This keeps checked/focus states inconsistent with the theme-variable objective.Suggested fix
input:checked + .slider { - background-color: `#2196f3`; + background-color: var(--ow-color-primary); } input:focus + .slider { - box-shadow: 0 0 5px 2px `#2196f3`; + box-shadow: 0 0 5px 2px var(--ow-color-primary); outline: none; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openwisp_notifications/static/openwisp-notifications/css/preferences.css` around lines 345 - 356, The checked/focus state colors are still hardcoded as `#2196f3`; update the rules that style the active knob/track (the selectors handling the checked and focus states—e.g., the input:checked + .slider and .slider:focus/slider:focus-visible rules that currently reference `#2196f3`) to use a theme CSS variable instead (for consistency with var(--border-color) and var(--ow-color-white) used elsewhere). Replace `#2196f3` with a semantic variable such as var(--ow-color-primary) (or var(--primary-color)) and include a fallback like var(--ow-color-primary, `#2196f3`) if you want to preserve the existing color when the variable is not defined.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@openwisp_notifications/static/openwisp-notifications/css/object-notifications.css`:
- Line 28: Replace the hardcoded background declaration "background: rgba(255,
255, 255, 0.5);" with the theme overlay variable so it follows the project's
design tokens—e.g. use "background: var(--ow-overlay-40);" (or another
appropriate --ow-overlay-* value matching intended opacity/contrast) or derive
from the white token like "background: rgba(var(--ow-color-white), 0.5)" if the
codebase supports CSS color variables; update the background property in
object-notifications.css where the rgba(...) appears.
In `@openwisp_notifications/static/openwisp-notifications/css/preferences.css`:
- Around line 238-239: The toast component still uses a hardcoded color for the
progress bar; update the `.toast .progress-bar` rule to use the shared theme
variable instead of `#007bff` (e.g., replace the hardcoded value with the
appropriate CSS variable such as `var(--ow-color-primary)` or
`var(--ow-color-accent)`), ensuring the `.toast` component fully follows the
theme migration and keeping contrast with `color: var(--ow-color-white)` intact.
---
Outside diff comments:
In `@openwisp_notifications/static/openwisp-notifications/css/preferences.css`:
- Around line 345-356: The checked/focus state colors are still hardcoded as
`#2196f3`; update the rules that style the active knob/track (the selectors
handling the checked and focus states—e.g., the input:checked + .slider and
.slider:focus/slider:focus-visible rules that currently reference `#2196f3`) to
use a theme CSS variable instead (for consistency with var(--border-color) and
var(--ow-color-white) used elsewhere). Replace `#2196f3` with a semantic variable
such as var(--ow-color-primary) (or var(--primary-color)) and include a fallback
like var(--ow-color-primary, `#2196f3`) if you want to preserve the existing color
when the variable is not defined.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f04e6518-4eea-4473-9d17-d0f06cfcbd9d
📒 Files selected for processing (4)
openwisp_notifications/static/openwisp-notifications/css/loader.cssopenwisp_notifications/static/openwisp-notifications/css/notifications.cssopenwisp_notifications/static/openwisp-notifications/css/object-notifications.cssopenwisp_notifications/static/openwisp-notifications/css/preferences.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). (6)
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
🔇 Additional comments (8)
openwisp_notifications/static/openwisp-notifications/css/loader.css (1)
9-22: LGTM!Also applies to: 33-33, 41-41
openwisp_notifications/static/openwisp-notifications/css/notifications.css (1)
25-25: LGTM!Also applies to: 29-29, 37-37, 43-43, 71-71, 75-75, 82-84, 87-87, 90-93, 96-96, 99-102, 141-141, 144-144, 149-149, 161-161, 164-166, 178-178, 182-184, 192-192, 200-200, 205-205, 209-209, 213-213, 217-217, 220-220, 230-230, 233-234, 302-302, 311-311, 319-319, 353-353, 356-356, 360-360
openwisp_notifications/static/openwisp-notifications/css/preferences.css (1)
37-45: LGTM!Also applies to: 60-63, 77-79, 138-141, 230-230, 314-315
openwisp_notifications/static/openwisp-notifications/css/object-notifications.css (5)
1-23: LGTM!
27-27: LGTM!
30-34: LGTM!
36-57: LGTM!
58-69: LGTM!
|
@nemesifier please review whenever free thanks! |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (2/3). |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (3/3). |
2 participants
Checklist
Reference to Existing Issue
Closes #425.
Description of Changes
replaced hardcoded CSS colors in notification UI with theme color variables
from openwisp-utils (defined in #516).