[feature] Add netjsongraph static files #517#675
Conversation
Added netjsongraph.echarts.min.js, netjsongraph.css, and netjsongraph-theme.css to the central static directory. Selected the unbundled ECharts variant to prevent redundant Leaflet JS loading, ensuring optimal compatibility with topology and monitoring modules. Fixes openwisp#517
|
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 adds centralized NetJSONGraph static assets to openwisp-utils for reuse across OpenWISP modules. It introduces two CSS stylesheets—netjsongraph.css and netjsongraph-theme.css—that define layout, loading animations, embedded icon font, Leaflet control styling, UI component layouts (sidebar, tooltips, controls), and responsive theme colors. Documentation in docs/developer/admin-theme.rst explains how other modules can include these assets via Django's {% static %} template tag. Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 6
🤖 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_utils/static/netjsongraph/css/netjsongraph-theme.css`:
- Around line 6-7: Review the use of !important in the theme rules (e.g.,
selectors like .njg-container .leaflet-control-zoom-in and .njg-container
.leaflet-control-zoom-out and the other rules referenced) and either remove the
!important by increasing selector specificity or scope (for example, prefix with
.njg-container and more specific leaf node selectors) so Leaflet defaults are
overridden without !important; if removal isn’t possible because Leaflet injects
inline or higher-specificity styles, keep the declarations but add a short
comment above those blocks stating that !important is required to override
Leaflet default styles and list the affected selectors for future maintainers.
- Around line 1-82: Extract the repeated theme color into CSS custom properties
on the .njg-container selector and use those variables where the hardcoded
rgba(217, 79, 52, 0.85) and repeated whites/greys are used; specifically add
properties like --theme-primary and --theme-white to .njg-container and replace
occurrences in selectors referenced in the diff (.leaflet-control-zoom-in,
.leaflet-control-zoom-out, .leaflet-control-layers, .sideBarHandle,
.njg-selectIcon, etc.) to reference the new variables so future theme changes
only require updating the custom properties.
- Around line 35-37: The checked checkbox label background uses grey that fails
WCAG AA contrast; update the CSS rule for the selector .njg-container
.switch-wrap input[type="checkbox"]:checked + label to use a darker color (e.g.
`#555555`) so the white text meets at least 4.5:1 contrast (WCAG AA); modify that
background property value accordingly and run a contrast check to confirm
compliance.
In `@openwisp_utils/static/netjsongraph/css/netjsongraph.css`:
- Line 172: The CSS rule uses a quoted generic font-family which Stylelint
rejects; update the font-family declarations (the lines containing font-family:
"iconfont"; and the repeated occurrence) to remove the quotes so they read
font-family: iconfont; ensuring both occurrences in netjsongraph.css are changed
to satisfy Stylelint.
- Line 301: Replace the deprecated declaration "word-break: break-word" with
modern wrapping rules: remove or change that property and add "overflow-wrap:
anywhere;" (and if needed keep "word-break: normal;") where "word-break:
break-word" appears in openwisp_utils/static/netjsongraph/css/netjsongraph.css
(the two occurrences around the reported lines). Ensure both occurrences are
updated so Stylelint passes and re-run ./run-qa-checks.
- Around line 1-5: The global rule ".njg-container * { outline: none; }" removes
keyboard focus outlines site-wide; remove that declaration and instead only
suppress outlines for specific non-keyboard interactive visuals if necessary
(target explicit selectors like buttons or inputs) and add a rule using
:focus-visible to preserve visible focus for keyboard users (e.g., keep or
define ".njg-container *:focus-visible { outline: ... }"); update the styles
around the .njg-container * selector and any custom button/input focus rules so
keyboard focus remains visible while allowing visual adjustments for mouse
focus.
🪄 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: f9fefce6-41ce-4a28-8c6c-ac86b7f9ab22
⛔ Files ignored due to path filters (1)
openwisp_utils/static/netjsongraph/js/netjsongraph.echarts.min.jsis excluded by!**/*.min.js
📒 Files selected for processing (2)
openwisp_utils/static/netjsongraph/css/netjsongraph-theme.cssopenwisp_utils/static/netjsongraph/css/netjsongraph.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). (14)
- 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.1.0
- GitHub Check: Python==3.10 | django~=5.0.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~=5.1.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{py,css,js,txt}
📄 CodeRabbit inference engine (AGENTS.md)
Run
./run-qa-checkswhich executesopenwisp-qa-checkwith project linter configuration for CSS, JS, and migration paths, and treat any failure as blocking
Files:
openwisp_utils/static/netjsongraph/css/netjsongraph-theme.cssopenwisp_utils/static/netjsongraph/css/netjsongraph.css
🪛 Stylelint (17.11.0)
openwisp_utils/static/netjsongraph/css/netjsongraph.css
[error] 301-301: Deprecated keyword "break-word" for property "word-break" (declaration-property-value-keyword-no-deprecated)
(declaration-property-value-keyword-no-deprecated)
[error] 306-306: Deprecated keyword "break-word" for property "word-break" (declaration-property-value-keyword-no-deprecated)
(declaration-property-value-keyword-no-deprecated)
[error] 172-172: Expected no quotes around "iconfont" (font-family-name-quotes)
(font-family-name-quotes)
[error] 186-186: Expected no quotes around "iconfont" (font-family-name-quotes)
(font-family-name-quotes)
🔇 Additional comments (1)
openwisp_utils/static/netjsongraph/css/netjsongraph-theme.css (1)
1-89: Ensure this CSS file passes the QA checks before merging.Per the project guidelines, CSS files must pass
./run-qa-checkswith the project's linter configuration. Run this check locally and resolve any linting violations before committing.
| .njg-container .leaflet-control-zoom-in, | ||
| .njg-container .leaflet-control-zoom-out { | ||
| pointer-events: auto; | ||
| opacity: 1; | ||
| cursor: pointer; | ||
| background: rgba(217, 79, 52, 0.85) !important; | ||
| color: #fff !important; | ||
| } | ||
|
|
||
| .njg-container .leaflet-control-zoom-in.leaflet-disabled, | ||
| .njg-container .leaflet-control-zoom-out.leaflet-disabled { | ||
| opacity: 0.7; | ||
| cursor: default; | ||
| } | ||
|
|
||
| .njg-container .leaflet-control-layers { | ||
| background: rgba(217, 79, 52, 0.85) !important; | ||
| color: #fff !important; | ||
| } | ||
|
|
||
| .njg-container .switch-wrap { | ||
| color: #fff; | ||
| } | ||
|
|
||
| .njg-container .switch-wrap label { | ||
| border: 1px solid #dfdfdf; | ||
| background: #ffffff; | ||
| } | ||
|
|
||
| .njg-container .switch-wrap label::before { | ||
| box-shadow: 0 1px 3px rgba(0, 0, 0, 1); | ||
| background-color: #fff; | ||
| } | ||
|
|
||
| .njg-container .switch-wrap input[type="checkbox"]:checked + label { | ||
| background: grey; | ||
| } | ||
|
|
||
| .njg-container .njg-date { | ||
| color: #fff; | ||
| } | ||
|
|
||
| .njg-container .njg-selectIcon { | ||
| background: rgba(217, 79, 52, 0.85); | ||
| color: #ffffff; | ||
| } | ||
|
|
||
| .njg-container .njg-searchBtn { | ||
| background-color: white; | ||
| } | ||
|
|
||
| .njg-container .njg-sideBar { | ||
| background-color: #fff; | ||
| } | ||
|
|
||
| .njg-container .sideBarHandle { | ||
| background: rgba(217, 79, 52, 0.85); | ||
| } | ||
|
|
||
| .njg-container .sideBarHandle::before { | ||
| color: #fff; | ||
| } | ||
|
|
||
| .njg-container .njg-metaData, | ||
| .njg-container .njg-infoContainer { | ||
| background-color: inherit; | ||
| } | ||
|
|
||
| .njg-container .njg-tooltip { | ||
| background: #fff !important; | ||
| border: none !important; | ||
| } | ||
|
|
||
| .njg-container .njg-tooltip-key, | ||
| .njg-container .njg-tooltip-value { | ||
| color: #000; | ||
| } | ||
|
|
||
| .njg-container .marker-cluster div { | ||
| color: #fff; | ||
| background-color: rgba(21, 102, 169, 0.8) !important; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider extracting repeated colors to CSS custom properties.
The primary theme color rgba(217, 79, 52, 0.85) is repeated 4 times (lines 6, 17, 44, 57). Using CSS custom properties would improve maintainability and make theme updates easier.
♻️ Refactor with CSS custom properties
Add at the top of the file:
.njg-container {
--theme-primary: rgba(217, 79, 52, 0.85);
--theme-white: `#ffffff`;
--theme-grey: `#555555`;
}Then replace the hardcoded values:
.njg-container .leaflet-control-zoom-in,
.njg-container .leaflet-control-zoom-out {
pointer-events: auto;
opacity: 1;
cursor: pointer;
- background: rgba(217, 79, 52, 0.85) !important;
- color: `#fff` !important;
+ background: var(--theme-primary) !important;
+ color: var(--theme-white) !important;
}Apply similar changes to lines 17, 44, and 57.
🤖 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_utils/static/netjsongraph/css/netjsongraph-theme.css` around lines 1
- 82, Extract the repeated theme color into CSS custom properties on the
.njg-container selector and use those variables where the hardcoded rgba(217,
79, 52, 0.85) and repeated whites/greys are used; specifically add properties
like --theme-primary and --theme-white to .njg-container and replace occurrences
in selectors referenced in the diff (.leaflet-control-zoom-in,
.leaflet-control-zoom-out, .leaflet-control-layers, .sideBarHandle,
.njg-selectIcon, etc.) to reference the new variables so future theme changes
only require updating the custom properties.
| background: rgba(217, 79, 52, 0.85) !important; | ||
| color: #fff !important; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Document or verify necessity of !important declarations.
Multiple rules use !important to override Leaflet styles. While this might be necessary for overriding external library defaults, consider whether increased specificity could achieve the same result without !important, which makes future overrides more difficult.
If !important is required for Leaflet overrides, consider adding a comment explaining this:
/* Using !important to override Leaflet default styles */
.njg-container .leaflet-control-zoom-in,
.njg-container .leaflet-control-zoom-out {
background: rgba(217, 79, 52, 0.85) !important;
color: `#fff` !important;
}Also applies to: 17-18, 70-71, 81-81
🤖 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_utils/static/netjsongraph/css/netjsongraph-theme.css` around lines 6
- 7, Review the use of !important in the theme rules (e.g., selectors like
.njg-container .leaflet-control-zoom-in and .njg-container
.leaflet-control-zoom-out and the other rules referenced) and either remove the
!important by increasing selector specificity or scope (for example, prefix with
.njg-container and more specific leaf node selectors) so Leaflet defaults are
overridden without !important; if removal isn’t possible because Leaflet injects
inline or higher-specificity styles, keep the declarations but add a short
comment above those blocks stating that !important is required to override
Leaflet default styles and list the affected selectors for future maintainers.
| .njg-container .switch-wrap input[type="checkbox"]:checked + label { | ||
| background: grey; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check WCAG color contrast for grey background with white text
python3 << 'EOF'
def relative_luminance(rgb):
"""Calculate relative luminance for RGB color"""
rgb = [x / 255.0 for x in rgb]
rgb = [(x / 12.92 if x <= 0.03928 else ((x + 0.055) / 1.055) ** 2.4) for x in rgb]
return 0.2126 * rgb[0] + 0.7152 * rgb[1] + 0.0722 * rgb[2]
def contrast_ratio(rgb1, rgb2):
"""Calculate contrast ratio between two colors"""
l1 = relative_luminance(rgb1)
l2 = relative_luminance(rgb2)
lighter = max(l1, l2)
darker = min(l1, l2)
return (lighter + 0.05) / (darker + 0.05)
# grey (`#808080`) vs white (`#ffffff`)
grey = [128, 128, 128]
white = [255, 255, 255]
ratio = contrast_ratio(grey, white)
print(f"Contrast ratio: {ratio:.2f}:1")
print(f"WCAG AA (normal text): {'PASS' if ratio >= 4.5 else 'FAIL'}")
print(f"WCAG AA (large text): {'PASS' if ratio >= 3 else 'FAIL'}")
print(f"WCAG AAA (normal text): {'PASS' if ratio >= 7 else 'FAIL'}")
EOFRepository: openwisp/openwisp-utils
Length of output: 174
🏁 Script executed:
# First, let's read the CSS file to understand the context around line 36
cat -n openwisp_utils/static/netjsongraph/css/netjsongraph-theme.css | head -50Repository: openwisp/openwisp-utils
Length of output: 1458
🏁 Script executed:
# Check if qa-checks script exists and run it as required by coding guidelines
if [ -f "./run-qa-checks" ]; then
./run-qa-checks -- openwisp_utils/static/netjsongraph/css/netjsongraph-theme.css
else
echo "run-qa-checks script not found"
fiRepository: openwisp/openwisp-utils
Length of output: 131
🏁 Script executed:
# Verify the proposed fix color (`#555555`) contrast ratio
python3 << 'EOF'
def relative_luminance(rgb):
"""Calculate relative luminance for RGB color"""
rgb = [x / 255.0 for x in rgb]
rgb = [(x / 12.92 if x <= 0.03928 else ((x + 0.055) / 1.055) ** 2.4) for x in rgb]
return 0.2126 * rgb[0] + 0.7152 * rgb[1] + 0.0722 * rgb[2]
def contrast_ratio(rgb1, rgb2):
"""Calculate contrast ratio between two colors"""
l1 = relative_luminance(rgb1)
l2 = relative_luminance(rgb2)
lighter = max(l1, l2)
darker = min(l1, l2)
return (lighter + 0.05) / (darker + 0.05)
# Proposed fix: `#555555` vs white (`#ffffff`)
proposed = [85, 85, 85]
white = [255, 255, 255]
ratio = contrast_ratio(proposed, white)
print(f"Proposed color (`#555555`) vs white:")
print(f"Contrast ratio: {ratio:.2f}:1")
print(f"WCAG AA (normal text): {'PASS' if ratio >= 4.5 else 'FAIL'}")
print(f"WCAG AA (large text): {'PASS' if ratio >= 3 else 'FAIL'}")
print(f"WCAG AAA (normal text): {'PASS' if ratio >= 7 else 'FAIL'}")
EOFRepository: openwisp/openwisp-utils
Length of output: 209
Use a darker background color to meet WCAG AA contrast requirements.
The checked checkbox label uses grey (#808080) as the background with white text, which produces a contrast ratio of 3.95:1. This fails to meet WCAG AA level requirements for normal text (4.5:1 minimum). Change to a darker color such as #555555, which achieves a 7.46:1 contrast ratio and satisfies WCAG AA and AAA standards.
Proposed fix
.njg-container .switch-wrap input[type="checkbox"]:checked + label {
- background: grey;
+ background: `#555555`;
}📝 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.
| .njg-container .switch-wrap input[type="checkbox"]:checked + label { | |
| background: grey; | |
| } | |
| .njg-container .switch-wrap input[type="checkbox"]:checked + label { | |
| background: `#555555`; | |
| } |
🤖 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_utils/static/netjsongraph/css/netjsongraph-theme.css` around lines
35 - 37, The checked checkbox label background uses grey that fails WCAG AA
contrast; update the CSS rule for the selector .njg-container .switch-wrap
input[type="checkbox"]:checked + label to use a darker color (e.g. `#555555`) so
the white text meets at least 4.5:1 contrast (WCAG AA); modify that background
property value accordingly and run a contrast check to confirm compliance.
| .njg-container * { | ||
| margin: 0; | ||
| padding: 0; | ||
| outline: none; | ||
| box-sizing: border-box; |
There was a problem hiding this comment.
Avoid removing all focus outlines globally.
outline: none on all descendants removes visible keyboard focus indicators and can block keyboard navigation.
Suggested fix
.njg-container * {
margin: 0;
padding: 0;
- outline: none;
box-sizing: border-box;
}
+
+.njg-container *:focus-visible {
+ outline: 2px solid currentColor;
+ outline-offset: 2px;
+}📝 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.
| .njg-container * { | |
| margin: 0; | |
| padding: 0; | |
| outline: none; | |
| box-sizing: border-box; | |
| .njg-container * { | |
| margin: 0; | |
| padding: 0; | |
| box-sizing: border-box; | |
| } | |
| .njg-container *:focus-visible { | |
| outline: 2px solid currentColor; | |
| outline-offset: 2px; | |
| } |
🤖 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_utils/static/netjsongraph/css/netjsongraph.css` around lines 1 - 5,
The global rule ".njg-container * { outline: none; }" removes keyboard focus
outlines site-wide; remove that declaration and instead only suppress outlines
for specific non-keyboard interactive visuals if necessary (target explicit
selectors like buttons or inputs) and add a rule using :focus-visible to
preserve visible focus for keyboard users (e.g., keep or define ".njg-container
*:focus-visible { outline: ... }"); update the styles around the .njg-container
* selector and any custom button/input focus rules so keyboard focus remains
visible while allowing visual adjustments for mouse focus.
| } | ||
|
|
||
| @font-face { | ||
| font-family: "iconfont"; |
There was a problem hiding this comment.
Fix font-family quoting to pass Stylelint.
Stylelint flags quoted generic font-family names here; this will fail QA checks for this PR.
Suggested fix
`@font-face` {
- font-family: "iconfont";
+ font-family: iconfont;
src: url("iconfont.eot?t=1554537125905"); /* IE9 */
...
}
...
.njg-container .iconfont {
- font-family: "iconfont", serif !important;
+ font-family: iconfont, serif !important;
font-size: 30px;
...
}As per coding guidelines, "**/*.{py,css,js,txt}: Run ./run-qa-checks ... and treat any failure as blocking".
Also applies to: 186-186
🧰 Tools
🪛 Stylelint (17.11.0)
[error] 172-172: Expected no quotes around "iconfont" (font-family-name-quotes)
(font-family-name-quotes)
🤖 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_utils/static/netjsongraph/css/netjsongraph.css` at line 172, The CSS
rule uses a quoted generic font-family which Stylelint rejects; update the
font-family declarations (the lines containing font-family: "iconfont"; and the
repeated occurrence) to remove the quotes so they read font-family: iconfont;
ensuring both occurrences in netjsongraph.css are changed to satisfy Stylelint.
| font-size: 14px; | ||
| font-weight: 600; | ||
| flex: 1 1 50%; | ||
| word-break: break-word; |
There was a problem hiding this comment.
Replace deprecated word-break: break-word values.
This value is deprecated and currently fails Stylelint; update to modern wrapping rules.
Suggested fix
.njg-container .njg-keyLabel {
font-size: 14px;
font-weight: 600;
flex: 1 1 50%;
- word-break: break-word;
+ word-break: normal;
+ overflow-wrap: anywhere;
}
.njg-container .njg-valueLabel {
flex: 1 1 50%;
- word-break: break-word;
+ word-break: normal;
+ overflow-wrap: anywhere;
}As per coding guidelines, "**/*.{py,css,js,txt}: Run ./run-qa-checks ... and treat any failure as blocking".
Also applies to: 306-306
🧰 Tools
🪛 Stylelint (17.11.0)
[error] 301-301: Deprecated keyword "break-word" for property "word-break" (declaration-property-value-keyword-no-deprecated)
(declaration-property-value-keyword-no-deprecated)
🤖 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_utils/static/netjsongraph/css/netjsongraph.css` at line 301, Replace
the deprecated declaration "word-break: break-word" with modern wrapping rules:
remove or change that property and add "overflow-wrap: anywhere;" (and if needed
keep "word-break: normal;") where "word-break: break-word" appears in
openwisp_utils/static/netjsongraph/css/netjsongraph.css (the two occurrences
around the reported lines). Ensure both occurrences are updated so Stylelint
passes and re-run ./run-qa-checks.
|
coverage: 97.488%. remained the same — kunalverma2512:feature/517-add-netjsongraph-static into openwisp:master |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
The issue being addressed (#517) states that we need to work on openwisp/openwisp-monitoring#709 as well, so most probably we need to address both issues at the same time and test both to make sure we're obtaining the result we want.
We need to move these files to openwisp_utils.admin_theme which is already listed in INSTALLED_APPS, otherwise the static finder won't find it (I believe openwisp_utils is not lsited in the INSTALLED_APPS.
The static files need to be in static/lib/netjsongraph.
We also need to add a mention of this in the developer docs page.
This PR on it's own cannot be merged until we have updated and verified all the affected modules.
|
Thanks for the review and pointers, @nemesifier I will move the files to openwisp_utils.admin_theme under static/lib/netjsongraph/ so the static finder can properly collect them, and I'll add a mention of this in the developer docs. Once I update this PR, I will set up openwisp-monitoring locally, update its templates to use these centralized files, and test everything together to ensure we successfully resolve both #517 and #709. I'll push the updates shortly! |
Relocated netjsongraph.js and related CSS files to the openwisp_utils.admin_theme static directory to ensure they are properly collected by the static finder. Updated admin-theme.rst documentation to reflect the new shared assets location. Fixes openwisp#517
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
openwisp_utils/admin_theme/static/lib/netjsongraph/css/netjsongraph.css (3)
301-301:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winReplace deprecated
word-break: break-wordvalue.The
break-wordkeyword forword-breakis deprecated. Useoverflow-wrap: anywhereinstead to achieve proper text wrapping.🔧 Use modern wrapping properties
.njg-container .njg-keyLabel { font-size: 14px; font-weight: 600; flex: 1 1 50%; - word-break: break-word; + word-break: normal; + overflow-wrap: anywhere; } .njg-container .njg-valueLabel { flex: 1 1 50%; - word-break: break-word; + word-break: normal; + overflow-wrap: anywhere; }As per coding guidelines, "
**/*.{py,css,js,txt}: Run./run-qa-checks... and treat any failure as blocking".Also applies to: 306-306
🤖 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_utils/admin_theme/static/lib/netjsongraph/css/netjsongraph.css` at line 301, The CSS uses the deprecated declaration "word-break: break-word"; replace each occurrence of that declaration with "overflow-wrap: anywhere" (and remove the deprecated "word-break: break-word" line) so modern browsers handle wrapping correctly—search for the literal string "word-break: break-word" in the stylesheet and change it to "overflow-wrap: anywhere" (keep any surrounding selectors/styles intact).
4-4:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove global
outline: noneto preserve keyboard accessibility.This declaration removes all focus indicators site-wide, blocking keyboard navigation and failing WCAG accessibility requirements.
♿ Proposed fix to preserve keyboard focus
.njg-container * { margin: 0; padding: 0; - outline: none; box-sizing: border-box; } + +.njg-container *:focus-visible { + outline: 2px solid currentColor; + outline-offset: 2px; +}🤖 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_utils/admin_theme/static/lib/netjsongraph/css/netjsongraph.css` at line 4, The global "outline: none" rule removes keyboard focus indicators and must be removed; locate the CSS rule containing "outline: none" in netjsongraph.css and delete it, or replace it by scoping focus suppression to specific mouse-only interactions (e.g., add a selector that targets only non-keyboard focus like :focus:not(:focus-visible) or a ".using-mouse :focus" helper) and ensure keyboard focus styles are preserved by using :focus-visible or explicit focus styles on interactive elements referenced in the file (e.g., buttons, links, .node, .edge) so keyboard users retain visible focus.
172-172:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove quotes from custom font-family name.
Stylelint requires custom font names to be unquoted. This error will cause QA checks to fail.
🔧 Fix font-family quoting
`@font-face` { - font-family: "iconfont"; + font-family: iconfont; src: url("iconfont.eot?t=1554537125905");As per coding guidelines, "
**/*.{py,css,js,txt}: Run./run-qa-checks... and treat any failure as blocking".Also applies to: 186-186
🤖 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_utils/admin_theme/static/lib/netjsongraph/css/netjsongraph.css` at line 172, The font-family declarations use quoted custom font name "iconfont" which fails stylelint; update the CSS rules that set font-family: "iconfont"; (and the duplicate at the other occurrence) to use an unquoted identifier font-family: iconfont; so the custom font name is unquoted and passes QA/stylelint.openwisp_utils/admin_theme/static/lib/netjsongraph/css/netjsongraph-theme.css (1)
35-37:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse darker background color to meet WCAG AA contrast requirements.
The
greybackground with white text produces a 3.95:1 contrast ratio, failing WCAG AA requirements (4.5:1 minimum). This blocks accessibility for users with vision impairments.♿ Proposed fix for contrast compliance
.njg-container .switch-wrap input[type="checkbox"]:checked + label { - background: grey; + background: `#555555`; }This achieves a 7.46:1 contrast ratio, meeting WCAG AA and AAA standards.
🤖 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_utils/admin_theme/static/lib/netjsongraph/css/netjsongraph-theme.css` around lines 35 - 37, The checked switch label uses the non-specific color "grey" which yields insufficient contrast; update the CSS rule for .njg-container .switch-wrap input[type="checkbox"]:checked + label to use a darker, WCAG-AA-compliant background color (e.g., replace "grey" with a hex value that achieves at least 4.5:1 contrast against white, such as a very dark gray/black) so the checked state meets accessibility contrast requirements.
🤖 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_utils/admin_theme/static/lib/netjsongraph/css/netjsongraph.css`:
- Around line 42-48: The .njg-chartContainer CSS rule contains a redundant
overflow property: remove the conflicting "overflow: hidden" declaration so only
"overflow-y: auto" remains; update the .njg-chartContainer block (the CSS rule
for class .njg-chartContainer) to eliminate the redundant line and keep width:
100%, height: 100%, z-index: 0, and overflow-y: auto.
- Around line 171-183: The `@font-face` block references missing icon font files
(iconfont.eot, iconfont.woff2/woff, iconfont.ttf, iconfont.svg) so the icon font
won't load; fix it by adding the actual font files with those exact names
(including the ?t= timestamp variant if you need cache-busting) into the same
static assets directory where netjsongraph.css lives or change the src URLs in
the `@font-face` declaration to point to the correct existing font
filenames/paths; ensure the filenames used in the `@font-face` (iconfont.eot,
iconfont.woff2/woff, iconfont.ttf, iconfont.svg) match the committed assets and
update any static manifest/build config if required.
- Line 72: The CSS contains a broken asset reference in the rule using the
declaration background: url("./loading.gif") center no-repeat; — fix it by
either adding the missing loading.gif at the same relative location used by that
URL or by removing/updating that background declaration to a valid asset or
fallback (for example remove the url(...) or replace with a data-URI or existing
spinner asset) so the loading indicator no longer references a non-existent
file.
---
Duplicate comments:
In
`@openwisp_utils/admin_theme/static/lib/netjsongraph/css/netjsongraph-theme.css`:
- Around line 35-37: The checked switch label uses the non-specific color "grey"
which yields insufficient contrast; update the CSS rule for .njg-container
.switch-wrap input[type="checkbox"]:checked + label to use a darker,
WCAG-AA-compliant background color (e.g., replace "grey" with a hex value that
achieves at least 4.5:1 contrast against white, such as a very dark gray/black)
so the checked state meets accessibility contrast requirements.
In `@openwisp_utils/admin_theme/static/lib/netjsongraph/css/netjsongraph.css`:
- Line 301: The CSS uses the deprecated declaration "word-break: break-word";
replace each occurrence of that declaration with "overflow-wrap: anywhere" (and
remove the deprecated "word-break: break-word" line) so modern browsers handle
wrapping correctly—search for the literal string "word-break: break-word" in the
stylesheet and change it to "overflow-wrap: anywhere" (keep any surrounding
selectors/styles intact).
- Line 4: The global "outline: none" rule removes keyboard focus indicators and
must be removed; locate the CSS rule containing "outline: none" in
netjsongraph.css and delete it, or replace it by scoping focus suppression to
specific mouse-only interactions (e.g., add a selector that targets only
non-keyboard focus like :focus:not(:focus-visible) or a ".using-mouse :focus"
helper) and ensure keyboard focus styles are preserved by using :focus-visible
or explicit focus styles on interactive elements referenced in the file (e.g.,
buttons, links, .node, .edge) so keyboard users retain visible focus.
- Line 172: The font-family declarations use quoted custom font name "iconfont"
which fails stylelint; update the CSS rules that set font-family: "iconfont";
(and the duplicate at the other occurrence) to use an unquoted identifier
font-family: iconfont; so the custom font name is unquoted and passes
QA/stylelint.
🪄 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: 24adb2cd-a901-4698-bb9e-b31207d5a707
⛔ Files ignored due to path filters (1)
openwisp_utils/admin_theme/static/lib/netjsongraph/js/netjsongraph.echarts.min.jsis excluded by!**/*.min.js
📒 Files selected for processing (3)
docs/developer/admin-theme.rstopenwisp_utils/admin_theme/static/lib/netjsongraph/css/netjsongraph-theme.cssopenwisp_utils/admin_theme/static/lib/netjsongraph/css/netjsongraph.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~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{py,css,js,txt}
📄 CodeRabbit inference engine (AGENTS.md)
Run
./run-qa-checkswhich executesopenwisp-qa-checkwith project linter configuration for CSS, JS, and migration paths, and treat any failure as blocking
Files:
openwisp_utils/admin_theme/static/lib/netjsongraph/css/netjsongraph-theme.cssopenwisp_utils/admin_theme/static/lib/netjsongraph/css/netjsongraph.css
🪛 Stylelint (17.11.0)
openwisp_utils/admin_theme/static/lib/netjsongraph/css/netjsongraph.css
[error] 301-301: Deprecated keyword "break-word" for property "word-break" (declaration-property-value-keyword-no-deprecated)
(declaration-property-value-keyword-no-deprecated)
[error] 306-306: Deprecated keyword "break-word" for property "word-break" (declaration-property-value-keyword-no-deprecated)
(declaration-property-value-keyword-no-deprecated)
[error] 172-172: Expected no quotes around "iconfont" (font-family-name-quotes)
(font-family-name-quotes)
[error] 186-186: Expected no quotes around "iconfont" (font-family-name-quotes)
(font-family-name-quotes)
🔇 Additional comments (2)
docs/developer/admin-theme.rst (1)
289-303: LGTM!openwisp_utils/admin_theme/static/lib/netjsongraph/css/netjsongraph.css (1)
218-220: ⚖️ Poor tradeoffThe
:has()selector passes linting and has no documented browser support restriction in this project.While
:has()is a modern CSS feature with limited support in older browsers (pre-2023 Safari/Chrome, not in IE11), the repository has no documented browser baseline or browserslist configuration that restricts it. The stylelint configuration will not flag:has()as an error, and the coding guidelines specify that QA check failures are blocking—this code does not fail any checks.If browser compatibility is a concern for this project, consider documenting the CSS baseline in browserslist or package.json rather than reviewing individual selector usage.
| .njg-chartContainer { | ||
| width: 100%; | ||
| height: 100%; | ||
| overflow: hidden; | ||
| z-index: 0; | ||
| overflow-y: auto; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Remove redundant overflow declaration.
Line 45 sets overflow: hidden, but line 47 immediately overrides it with overflow-y: auto, making the first declaration ineffective.
♻️ Simplify by removing redundant line
.njg-chartContainer {
width: 100%;
height: 100%;
- overflow: hidden;
z-index: 0;
overflow-y: auto;
}📝 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.
| .njg-chartContainer { | |
| width: 100%; | |
| height: 100%; | |
| overflow: hidden; | |
| z-index: 0; | |
| overflow-y: auto; | |
| } | |
| .njg-chartContainer { | |
| width: 100%; | |
| height: 100%; | |
| z-index: 0; | |
| overflow-y: auto; | |
| } |
🤖 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_utils/admin_theme/static/lib/netjsongraph/css/netjsongraph.css`
around lines 42 - 48, The .njg-chartContainer CSS rule contains a redundant
overflow property: remove the conflicting "overflow: hidden" declaration so only
"overflow-y: auto" remains; update the .njg-chartContainer block (the CSS rule
for class .njg-chartContainer) to eliminate the redundant line and keep width:
100%, height: 100%, z-index: 0, and overflow-y: auto.
| margin: 0 auto; | ||
| width: 40px; | ||
| height: 40px; | ||
| background: url("./loading.gif") center no-repeat; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if loading.gif exists in the expected location
fd -t f "loading.gif" openwisp_utils/admin_theme/static/lib/netjsongraph/Repository: openwisp/openwisp-utils
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Check what files exist in the css directory
echo "=== Files in CSS directory ==="
find openwisp_utils/admin_theme/static/lib/netjsongraph/css/ -type f
echo -e "\n=== Search for any .gif files in the netjsongraph directory ==="
find openwisp_utils/admin_theme/static/lib/netjsongraph/ -name "*.gif" -type f
echo -e "\n=== Search for loading.gif anywhere in the repository ==="
find . -name "loading.gif" -type f 2>/dev/null || echo "No loading.gif found"Repository: openwisp/openwisp-utils
Length of output: 372
🏁 Script executed:
#!/bin/bash
# Run QA checks per coding guidelines for CSS files
./run-qa-checks openwisp_utils/admin_theme/static/lib/netjsongraph/css/netjsongraph.cssRepository: openwisp/openwisp-utils
Length of output: 131
Add missing loading.gif asset or remove the broken reference.
The stylesheet references ./loading.gif at line 72 for the loading animation, but this file does not exist in the repository. Either add the missing asset file at openwisp_utils/admin_theme/static/lib/netjsongraph/css/loading.gif or update the CSS to remove this broken reference, as the loading indicator will not display without it.
🤖 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_utils/admin_theme/static/lib/netjsongraph/css/netjsongraph.css` at
line 72, The CSS contains a broken asset reference in the rule using the
declaration background: url("./loading.gif") center no-repeat; — fix it by
either adding the missing loading.gif at the same relative location used by that
URL or by removing/updating that background declaration to a valid asset or
fallback (for example remove the url(...) or replace with a data-URI or existing
spinner asset) so the loading indicator no longer references a non-existent
file.
| @font-face { | ||
| font-family: "iconfont"; | ||
| src: url("iconfont.eot?t=1554537125905"); /* IE9 */ | ||
| src: | ||
| url("iconfont.eot?t=1554537125905#iefix") format("embedded-opentype"), | ||
| /* IE6-IE8 */ | ||
| url("data:application/x-font-woff2;charset=utf-8;base64,d09GMgABAAAAAALUAAsAAAAABoQAAAKGAAEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAHEIGVgCCcAqBEIEuATYCJAMICwYABCAFhG0HLRvRBcguoRzbUaRIzQw73HPLf7QTlBZ2D/IcIqjWsJ69DaJClJGADlmhijCpCBUFUgELGaHg/4X/3P9am73DJNR5j3rniUuYJZWV+Sv+d1DrkBIhatJGhdBMSonZDe5mGYwm3qYronsgVPRfaegmrtP2H/dOG0u7QOYDymWugZ+6AOOtgY41KLISyrjHcAqrwkXmf5xApQ4ZykLfyDjUyWhcIC5sU4W6gk+W5WS5UNqwNItbJcrTdQrcRN+Pf0pRR1JSoLK1814DOr6Kp7H/u+8oQPETcHUJCowBmdhqjK1IC5mSVhmr0gOOVRx81QcGcVStkP86KqwGbcB7Jr4neNTCBDL0jgKLk9zkWpoZV083Lk3aU+QEjguciAOc6rXwcih7x22pDzGBTA58/6i7Kf9X4OyAnUy7Lf+SwMWzebmVOkI2slFyqJCVI4UN6dZul2us9Ehnd6SGqrxTzKCfFY+pPRQE/8sGl1onvrwqg8/d6kiTDXOA8kvZg7+MP7AmW0JSppPz0J6M5rIkVFpOv8jV6GWqquX5LKG8wJchKdMOhXJ96IwfgxJVxqFUuWmoNKp/dZUmUlrkCow4YRDqnUNS6x4K9R7RGf8MJVq9Qqn6SEOlnWjasspA2A3zklQmA7UNtPUwEK6RG+LiUTJXPFXmhRF1kmRiB7GYK5ST/RSQXGJGsmqWmAUKGfrYx04jzwsxkqFDOucs5qiSz4umN+X00IcG8ySiYsSANBuQTRcKiKAxa6h8fhQxrfCoZAeiyTqJSAm7f1SUUxiA7pcFgxCP8kpilamEMQEJUsiH+tgg4vGEUNQ8yEF0LMeaEIlU5Hk9MVSW217h/7pNUInUKXhVobhUAwAAAA==") | ||
| format("woff2"), | ||
| url("iconfont.woff?t=1554537125905") format("woff"), | ||
| url("iconfont.ttf?t=1554537125905") format("truetype"), | ||
| /* chrome, firefox, opera, Safari, Android, iOS 4.2+ */ | ||
| url("iconfont.svg?t=1554537125905#iconfont") format("svg"); /* iOS 4.1- */ | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for icon font files
fd -t f "iconfont\.(eot|woff|ttf|svg)" openwisp_utils/admin_theme/static/lib/netjsongraph/Repository: openwisp/openwisp-utils
Length of output: 49
🏁 Script executed:
#!/bin/bash
# List all files in the netjsongraph directory
echo "=== Files in netjsongraph directory ==="
find openwisp_utils/admin_theme/static/lib/netjsongraph -type f | sort
echo -e "\n=== Search for any iconfont files in the entire repository ==="
find openwisp_utils -name "iconfont*" -type f 2>/dev/null | sort
echo -e "\n=== Check .gitignore for font-related patterns ==="
if [ -f .gitignore ]; then
grep -E "\.(eot|woff|ttf|svg|woff2)" .gitignore || echo "No font-related patterns found in .gitignore"
fiRepository: openwisp/openwisp-utils
Length of output: 500
🏁 Script executed:
#!/bin/bash
# Run the required QA checks per coding guidelines
if [ -f "./run-qa-checks" ]; then
./run-qa-checks openwisp_utils/admin_theme/static/lib/netjsongraph/css/netjsongraph.css
else
echo "run-qa-checks script not found"
fiRepository: openwisp/openwisp-utils
Length of output: 131
Icon font files referenced in @font-face are missing.
The CSS declaration at lines 171-183 references four icon font files (iconfont.eot, iconfont.woff, iconfont.ttf, iconfont.svg) that do not exist in the repository. These files must be added to openwisp_utils/admin_theme/static/lib/netjsongraph/ for the font face to load correctly at runtime.
🧰 Tools
🪛 Stylelint (17.11.0)
[error] 172-172: Expected no quotes around "iconfont" (font-family-name-quotes)
(font-family-name-quotes)
🤖 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_utils/admin_theme/static/lib/netjsongraph/css/netjsongraph.css`
around lines 171 - 183, The `@font-face` block references missing icon font files
(iconfont.eot, iconfont.woff2/woff, iconfont.ttf, iconfont.svg) so the icon font
won't load; fix it by adding the actual font files with those exact names
(including the ?t= timestamp variant if you need cache-busting) into the same
static assets directory where netjsongraph.css lives or change the src URLs in
the `@font-face` declaration to point to the correct existing font
filenames/paths; ensure the filenames used in the `@font-face` (iconfont.eot,
iconfont.woff2/woff, iconfont.ttf, iconfont.svg) match the committed assets and
update any static manifest/build config if required.
ReStructuredText Formatting ErrorsHello @kunalverma2512, The CI is failing due to ReStructuredText formatting errors in the Fix: Please run the |
Adjusted line lengths and spacing in admin-theme.rst to comply with the project's ReStructuredText style guidelines and pass CI formatting checks. Fixes openwisp#517
ReStructuredText Formatting ErrorsHello @kunalverma2512, The CI failed because the ReStructuredText file Fix: |
Added a blank line at the end of the documentation file to satisfy the 'Blank endline' check in the CI pipeline. Fixes openwisp#517
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
ReStructuredText Formatting ErrorsHello @kunalverma2512, The CI is failing due to ReStructuredText formatting errors in the Fix: To resolve this, please run the following command in your local environment: openwisp-qa-formatThis command will automatically format the ReStructuredText file according to the project's standards. After formatting, please commit the changes and push them again. |
Updated section hierarchy to Level 3, matched underline length, and corrected code block indentation to pass CI checks. Fixes openwisp#517
ReStructuredText Formatting FailureHello @kunalverma2512, The CI failed because the ReStructuredText file To fix this, please run the |
…#517 Applied official OpenWISP formatting rules to the documentation to resolve CI linting errors. Fixes openwisp#517
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/developer/admin-theme.rst`:
- Around line 300-305: The docs reference a non-existent asset
"lib/netjsongraph/js/netjsongraph.echarts.min.js" via the <script> tag shown in
the example; either add the missing JavaScript asset into the project's static
assets under the admin theme static tree (so it is served and collectstatic will
include it) or remove the <script src="...netjsongraph.echarts.min.js"> tag from
the example and update the example to only include the existing CSS/JS files
(netjsongraph.css and netjsongraph-theme.css) to avoid a 404 in the
documentation.
🪄 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: bc458135-7440-436f-b291-9d3ec906b44f
📒 Files selected for processing (1)
docs/developer/admin-theme.rst
📜 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). (14)
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.13 | django~=5.1.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.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.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
🔇 Additional comments (1)
docs/developer/admin-theme.rst (1)
290-298: LGTM!
|
Hi @nemesifier, thank you for the detailed feedback. I have updated this PR to address the architectural requirements you mentioned:
I fully understand that this is only the first half of the work. To address openwisp-monitoring#709 and ensure we are avoiding redundant Leaflet loading, I am now setting up the My next step is to update the templates in those modules to load the centralized assets from Any further guidance you can provide to ensure I’m aligning perfectly with the project’s integration goals would be greatly appreciated. It will help me get these changes finalized as nicely as possible! |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Thanks @kunalverma2512, looks on the right track.
Let's link this PRs to the other PRs you have opened that are related to this. See also my comments below.
We need to verify all the PRs as if it was only one: there's no point in merging just one change, we need to verify, test and merge all of them at the same time, starting from this one and then the other ones once we have verified everything works as expected.
| ~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| ``openwisp-utils`` provides the unbundled ECharts variant of | ||
| ``netjsongraph.js`` (and its base CSS themes) so it can be reused across |
There was a problem hiding this comment.
Instead of flagging this as code, please link the github repo and call this "the netjsongraph.js library", let's also write that other modules use this library: monitoring (for the geographic and indoor map) and network-topology for the topology graph, let's also add a link to those modules, eg:
:doc:`/monitoring/geo-indoor-maps` # this should add a link to https://openwisp.io/docs/dev/monitoring/user/geo-indoor-maps.html
:doc:`/network-topology/index` # this should add a link to https://openwisp.io/docs/dev/network-topology/
```.
| <https://github.com/openwisp/openwisp-utils/blob/master/openwisp_utils/admin_theme/templates/openwisp_utils/email_template.html>`_ | ||
| for reference implementation. | ||
|
|
||
| Shared NetJSONGraph Assets |
There was a problem hiding this comment.
| Shared NetJSONGraph Assets | |
| Shared Netjsongraph.js Assets |
|
|
||
| ``openwisp-utils`` provides the unbundled ECharts variant of | ||
| ``netjsongraph.js`` (and its base CSS themes) so it can be reused across | ||
| different OpenWISP modules without redundantly bundling Leaflet JS. |
There was a problem hiding this comment.
| different OpenWISP modules without redundantly bundling Leaflet JS. | |
| different OpenWISP modules without redundantly bundling Leaflet JS (which is already shipped and loaded by django-leaflet). |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Looks on the right track but some important details are missing in regards to manual testing not sufficiently demonstrated.
See my comments below and the reviews in:
openwisp/openwisp-network-topology#298
openwisp/openwisp-monitoring#794
Commit Message Format FailureHello @kunalverma2512, The CI failed because the commit message does not follow the required format. Specifically, the issue number Fix: Example of correct format: |
Updated admin-theme.rst to accurately describe the unbundled netjsongraph.js library and its usage across OpenWISP modules. Replaced the previous description with the maintainer-approved wording, including proper GitHub links and cross-references to the monitoring and network-topology documentation. Fixes openwisp#517
kunalverma2512
force-pushed
the
feature/517-add-netjsongraph-static
branch
from
b8fe1bc to
19bba45
Compare
Added the missing echarts.min.js (v6.0.0) file to the unbundled static library directory. This ensures the ECharts library is properly loaded when requested by downstream modules like openwisp-monitoring and openwisp-network-topology, resolving the 404 Not Found error in the browser UI. Fixes openwisp#517
|
Hi @nemesifier, I am done with the work. I have applied your suggested changes. I have also completed the manual testing across all three modules. All 3 PRs are now giving green checks, and I have attached the proper screenshots to the other PRs proving that the unbundled files load correctly and the UI works as expected. Let me know if everything looks good here! |
3 participants
Added netjsongraph.echarts.min.js, netjsongraph.css, and netjsongraph-theme.css to the central static directory. Selected the unbundled ECharts variant to prevent redundant Leaflet JS loading, ensuring optimal compatibility with topology and monitoring modules.
Checklist
Reference to Existing Issue
Closes #517
Description of Changes
Hi @nemesifier! I have created this draft PR to centralize the
netjsongraph.jsfiles intoopenwisp-utils.As requested in the issue dependencies, I used the
netjsongraph.echarts.min.jsvariant to ensure we avoid the redundant Leaflet JS bug. I also included the base CSS and theme CSS. I placed them inopenwisp_utils/static/netjsongraph/.Could you please review if this is the correct approach and directory structure?
Screenshot
(N/A - Added backend static files, no UI changes yet)