Skip to content
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .nvmrc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
18
11 changes: 7 additions & 4 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -247,12 +247,15 @@ NetJSON format used internally is based on [networkgraph](http://netjson.org/rfc

Whether to allow switching between graph and map render or not. You can also set it `true` to enable it.

- `showLabelsAtZoomLevel`
- `showMapLabelsAtZoom`

**Default**: `7`
**Default**: `13`

The zoom level at which the labels are shown. This only works when `render` is set to `map`.
In graph mode, the overlapping labels are hidden automatically when zooming.
Controls when map labels are shown. This only works when `render` is set to `map`.
- If set to `false`, labels are completely disabled and will never be shown.
- If set to a number (e.g., `13`), labels will be shown when the map zoom level is greater than or equal to that value.

In graph mode, the overlapping labels are hidden automatically when zooming (use `showGraphLabelsAtZoom` for graph mode).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove this line as it mentions the next config option which is treated below, let's not mix things.


- `showGraphLabelsAtZoom`

Expand Down
2 changes: 1 addition & 1 deletion src/js/netjsongraph.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ const NetJSONGraphDefaultConfig = {
clusterRadius: 80,
clusterSeparation: 20,
showMetaOnNarrowScreens: false,
showLabelsAtZoomLevel: 13,
showMapLabelsAtZoom: 13,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an unsolicited backward incompatible renaming

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged. I am working on the fixes now. I will revert showMapLabelsAtZoom back to showLabelsAtZoomLevel and remove the unrelated formatting changes. I will force push the updates shortly to clean up the commit history.

Copy link
Member

@nemesifier nemesifier Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I was wrong about this comment, we need to read the issue description again more closely. I am sorry for this mistake, I have too many things on my mind right now and I can forget some details, let's follow the issue description and reply to the issues brought up there and use it as a guide to resolve the issue. That's the right way to do it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From #454:

(in the code we can continue supporting showLabelsAtZoomLevel for backward compatibility)

Is this being honored and if yes how?

showGraphLabelsAtZoom: null,
crs: L.CRS.EPSG3857,
echartsOption: {
Expand Down
6 changes: 6 additions & 0 deletions src/js/netjsongraph.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,12 @@ class NetJSONGraph {
this.setupGraph();
this.config.onInit.call(this.graph);
this.initializeECharts();
if (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (
// Maintain backward compatibility with old config option "showLabelsAtZoomLevel"
// TODO: remove in future versions
if (

this.config.showMapLabelsAtZoom === undefined &&
this.config.showLabelsAtZoomLevel !== undefined
) {
this.config.showMapLabelsAtZoom = this.config.showLabelsAtZoomLevel;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
this.config.showMapLabelsAtZoom = this.config.showLabelsAtZoomLevel;
console.warn("showLabelsAtZoomLevel has been renamed to showMapLabelsAtZoom, please update your code accordingly.")
this.config.showMapLabelsAtZoom = this.config.showLabelsAtZoomLevel;

}
// eslint-disable-next-line no-constructor-return
return this.graph;
}
Expand Down
69 changes: 61 additions & 8 deletions src/js/netjsongraph.render.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ class NetJSONGraphRender {

tooltip: {
confine: true,
hideDelay: 0,
position: (pos, params, dom, rect, size) => {
let position = "right";
if (size.viewSize[0] - pos[0] < size.contentSize[0]) {
Expand Down Expand Up @@ -179,6 +180,9 @@ class NetJSONGraphRender {
const baseGraphSeries = {...configs.graphConfig.series};
const baseGraphLabel = {...(baseGraphSeries.label || {})};

// Added this for label hover issue
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is too vague... what issue? We have lots of issues with labels. What does this solve exactly?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Added this for label hover issue
// Prevent redundant overlapping labels

baseGraphLabel.silent = true;

// Shared helper to get current graph zoom level
const getGraphZoom = () => {
try {
Expand Down Expand Up @@ -329,7 +333,6 @@ class NetJSONGraphRender {
});

nodesData = nodesData.concat(clusters);

const series = [
{
id: "geo-map",
Expand All @@ -341,7 +344,11 @@ class NetJSONGraphRender {
coordinateSystem: "leaflet",
data: nodesData,
animationDuration: 1000,
label: configs.mapOptions.nodeConfig.label,
label: {
...(configs.mapOptions.nodeConfig.label || {}),
...(!configs.showMapLabelsAtZoom ? {show: false} : {}),
silent: true,
},
Comment on lines +347 to +351
Copy link

@coderabbitai coderabbitai bot Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Treat showMapLabelsAtZoom = 0 as a valid threshold.

Using !showMapLabelsAtZoom treats 0 as “disabled,” which can unintentionally hide labels at zoom 0. Prefer explicit checks for false/null/undefined and treat numeric thresholds (including 0) as valid.

🔧 Suggested fix
-        label: {
-          ...(configs.mapOptions.nodeConfig.label || {}),
-          ...(!configs.showMapLabelsAtZoom ? {show: false} : {}),
-          silent: true,
-        },
+        label: {
+          ...(configs.mapOptions.nodeConfig.label || {}),
+          ...((configs.showMapLabelsAtZoom === false ||
+            configs.showMapLabelsAtZoom === undefined ||
+            configs.showMapLabelsAtZoom === null)
+            ? {show: false}
+            : {}),
+          silent: true,
+        },
-    const {showMapLabelsAtZoom} = self.config;
-    if (
-      !showMapLabelsAtZoom ||
-      (self.leaflet && self.leaflet.getZoom() < showMapLabelsAtZoom)
-    ) {
+    const {showMapLabelsAtZoom} = self.config;
+    const labelsDisabled =
+      showMapLabelsAtZoom === false ||
+      showMapLabelsAtZoom === undefined ||
+      showMapLabelsAtZoom === null;
+    const hasThreshold = typeof showMapLabelsAtZoom === "number";
+    const showLabel =
+      !labelsDisabled &&
+      (!hasThreshold || (self.leaflet && self.leaflet.getZoom() >= showMapLabelsAtZoom));
+    if (!showLabel) {
       self.echarts.setOption({
         series: [
           {
             id: "geo-map",
             label: {
               show: false,
               silent: true,
             },
             emphasis: {
               label: {
                 show: false,
               },
             },
           },
         ],
       });
     }
 
     self.echarts.on("mouseover", () => {
       // FIX: Removed the variable declaration. We use the one from upper scope.
       if (
-        showMapLabelsAtZoom &&
+        !labelsDisabled &&
+        hasThreshold &&
         self.leaflet &&
         self.leaflet.getZoom() >= showMapLabelsAtZoom
       ) {
         self.echarts.setOption({
           series: [
             {
               id: "geo-map",
               label: {
                 show: false,
                 silent: true,
               },
             },
           ],
         });
       }
     });
 
     self.echarts.on("mouseout", () => {
       if (
-        showMapLabelsAtZoom &&
+        !labelsDisabled &&
+        hasThreshold &&
         self.leaflet &&
         self.leaflet.getZoom() >= showMapLabelsAtZoom
       ) {
         self.echarts.setOption({
           series: [
             {
               id: "geo-map",
               label: {
                 show: true,
                 silent: true,
               },
             },
           ],
         });
       }
     });
 
     self.leaflet.on("zoomend", () => {
       const currentZoom = self.leaflet.getZoom();
-      const show = showMapLabelsAtZoom && currentZoom >= showMapLabelsAtZoom;
+      const show =
+        !labelsDisabled && (!hasThreshold || currentZoom >= showMapLabelsAtZoom);
       self.echarts.setOption({
         series: [
           {
             id: "geo-map",
             label: {
               show,
               silent: true,
             },
             emphasis: {
               label: {
                 show: false,
               },
             },
           },
         ],
       });

Also applies to: 556-568, 579-618, 622-634

🤖 Prompt for AI Agents
In `@src/js/netjsongraph.render.js` around lines 347 - 351, The label show/hide
logic incorrectly treats 0 as falsy by using "!configs.showMapLabelsAtZoom";
change it to explicitly check for null/undefined/false so numeric thresholds
(including 0) remain valid. Locate the label merge where you see
"...(!configs.showMapLabelsAtZoom ? {show: false} : {})" and replace that
condition with an explicit check like "configs.showMapLabelsAtZoom === false ||
configs.showMapLabelsAtZoom == null ? {show: false} : {}"; apply the same fix to
the other similar blocks referenced (around the other occurrences noted).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since showing labels at zoom 0 (world view) isn't a practical use case for us, we are sticking with the simpler syntax.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR description image is not visible in this thread, so I can’t describe it. No Comment image was provided.

@coderabbit-user Thanks for confirming. Acknowledged: we’ll keep the simpler falsy checks. For clarity going forward, here’s the effective behavior today:

  • Any falsy value (false, null, undefined, or 0) for showMapLabelsAtZoom disables map labels; otherwise a number enables labels at or above that zoom.

Optional low-effort follow-ups to prevent confusion later:

  • README note: clarify that 0 is treated as “disabled” and is not a supported threshold.
  • Tiny test: assert that showMapLabelsAtZoom = 0 keeps labels hidden at all zooms.
  • Optional normalization (makes intent explicit at config load):
// src/js/netjsongraph.js (constructor or config normalization)
if (config.showMapLabelsAtZoom === 0) {
  config.showMapLabelsAtZoom = false;
}

If you want, I can prep a small doc + test patch in this PR or as a follow-up. Otherwise, I’m good with your decision and won’t block on this.


✏️ Learnings added
Learnt from: Dhruv-ub
Repo: openwisp/netjsongraph.js PR: 474
File: src/js/netjsongraph.render.js:347-351
Timestamp: 2026-01-30T08:31:13.746Z
Learning: Project preference: treat showMapLabelsAtZoom = 0 as “disabled” (not a valid threshold) and keep falsy checks in src/js/netjsongraph.render.js; document or normalize this if needed.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
Learnt from: codesankalp
Repo: openwisp/netjsongraph.js PR: 425
File: src/js/netjsongraph.render.js:1-26
Timestamp: 2026-01-20T16:51:06.213Z
Learning: In the netjsongraph.js project, imports from `echarts/lib/chart/*/install`, `echarts/lib/component/*/install`, and `echarts/lib/renderer/install*` paths produce better tree-shaking results than the public entry points (`echarts/charts`, `echarts/components`, `echarts/renderers`) in their webpack configuration. The project maintainer has tested both approaches and confirmed the `/lib/*/install` pattern yields smaller bundles.

itemStyle: {
color: (params) => {
if (
Expand Down Expand Up @@ -546,13 +553,18 @@ class NetJSONGraphRender {
}
}

if (self.leaflet.getZoom() < self.config.showLabelsAtZoomLevel) {
const {showMapLabelsAtZoom} = self.config;
if (
!showMapLabelsAtZoom ||
(self.leaflet && self.leaflet.getZoom() < showMapLabelsAtZoom)
) {
self.echarts.setOption({
series: [
{
id: "geo-map",
label: {
show: false,
silent: true,
},
emphasis: {
label: {
Expand All @@ -564,25 +576,66 @@ class NetJSONGraphRender {
});
}

self.echarts.on("mouseover", () => {
// FIX: Removed the variable declaration. We use the one from upper scope.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// FIX: Removed the variable declaration. We use the one from upper scope.

if (
showMapLabelsAtZoom &&
self.leaflet &&
self.leaflet.getZoom() >= showMapLabelsAtZoom
) {
self.echarts.setOption({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this have to change based on the current zoom level? Shouldn't the zoom level be evaluated in this function? What if the mouseover event happens after map rendering and the user has zoomed in or out?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I also noticed this regarding the dynamic zoom check—I'll update the handler to evaluate that live.

series: [
{
id: "geo-map",
label: {
show: false,
silent: true,
},
},
],
});
}
});

self.echarts.on("mouseout", () => {
if (
showMapLabelsAtZoom &&
self.leaflet &&
self.leaflet.getZoom() >= showMapLabelsAtZoom
) {
self.echarts.setOption({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same question here

series: [
{
id: "geo-map",
label: {
show: true,
silent: true,
},
},
],
});
}
});

self.leaflet.on("zoomend", () => {
const currentZoom = self.leaflet.getZoom();
const showLabel = currentZoom >= self.config.showLabelsAtZoomLevel;
const show = showMapLabelsAtZoom && currentZoom >= showMapLabelsAtZoom;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the showLabel naming here was better.

self.echarts.setOption({
series: [
{
id: "geo-map",
label: {
show: showLabel,
show,
silent: true,
},
emphasis: {
label: {
show: showLabel,
show: false,
},
},
},
],
});

// Zoom in/out buttons disabled only when it is equal to min/max zoomlevel
// Manually handle zoom control state to ensure correct behavior with float zoom levels
const minZoom = self.leaflet.getMinZoom();
Expand Down Expand Up @@ -686,9 +739,9 @@ class NetJSONGraphRender {
params.componentSubType === "effectScatter") &&
params.data.cluster
) {
// Zoom into the clicked cluster instead of expanding it
const currentZoom = self.leaflet.getZoom();
const targetZoom = Math.min(currentZoom + 2, self.leaflet.getMaxZoom());

self.leaflet.setView(
[params.data.value[1], params.data.value[0]],
targetZoom,
Expand Down
Loading