netjsongraph.js icon indicating copy to clipboard operation
netjsongraph.js copied to clipboard

[bug] Node labels and overlays are redundant

Open nemesifier opened this issue 2 months ago • 4 comments

Image

When zooming in, this is clearly visible in:

  • https://openwisp.github.io/netjsongraph.js/examples/netjsonmap.html
  • https://openwisp.github.io/netjsongraph.js/examples/netjson-clustering.html

I think we need to ensure that by default these two do not overlap.

I don't have a clear directive now, but I think the whole situation is a mess, here's the problems I see, I make suggestions at each point but I am open for better ideas:

  • I am not sure where the hover overlay is coming from, whether is enabled by default or not
  • It seems to me there's not an explicit ways to disable labels on the geographic map (they appears automatically after a certain level of zoom)
  • If both are enabled, the overlap covers the label and they're quite redundant, ideally the labels should disappear if there's another overlay showing up on hover and by default only one of the two should be enabled (probably the label as it's not invasive)
  • The current config options for controlling this behavior are: showLabelsAtZoomLevel for geo map and showGraphLabelsAtZoom, which is not consistent, I think we should rename showLabelsAtZoomLevel to showMapLabelsAtZoom (in the code we can continue supporting showLabelsAtZoomLevel for backward compatibility) and allow this to be false to disable lables altogether

nemesifier avatar Oct 15 '25 18:10 nemesifier

Hi! I’ve gone through the issue and would like to take it up.
I’ve started looking into a possible solution.
Kindly assign this issue to me so I can proceed with implementation.

durgax04 avatar Oct 16 '25 07:10 durgax04

Thanks for your interest @durgax04! Feel free to open a PR.

You don't need to wait for the issue to be assigned to you. Just check if there is anyone else actively working on it (e.g.: an open pull request with recent activity). If nobody else is actively working on it, just announce your intention to work on it by leaving a comment in the issue.

Quoted from our Contributing Guidelines, please have a read.

nemesifier avatar Oct 16 '25 12:10 nemesifier

I’m sorry if my previous pull request (#454) wasn’t up to the project’s expectations — this was my first contribution, and I may have made some mistakes.

I genuinely want to learn and improve my approach. Could you please guide me on how I can make a better fix for this issue or what areas I should focus on?

durgax04 avatar Oct 20 '25 07:10 durgax04

@nemesifier I’ve removed showLabelsAtZoomLevel from the default config and added backward compatibility logic in netjsongraph.render.js to ensure older configs still work.

const NetJSONGraphDefaultConfig = {
   clusterRadius: 80,
   clusterSeparation: 20,
   showMetaOnNarrowScreens: false,
-  showLabelsAtZoomLevel: 13,
   showGraphLabelsAtZoom: null,
   crs: L.CRS.EPSG3857,
   echartsOption: {

netjsongraph.render.js

-    if (self.leaflet.getZoom() < self.config.showLabelsAtZoomLevel) {
+    const showMapLabelsAtZoom =
+      self.config.showMapLabelsAtZoom !== undefined
+        ? self.config.showMapLabelsAtZoom
+        : self.config.showLabelsAtZoomLevel;
+
+    if (self.leaflet.getZoom() < showMapLabelsAtZoom) {
-      const showLabel = currentZoom >= self.config.showLabelsAtZoomLevel;
+      const showLabel = currentZoom >= showMapLabelsAtZoom;

This change makes the option name consistent (showMapLabelsAtZoom) and keeps support for the old one (showLabelsAtZoomLevel) for backward compatibility.

Could you please confirm if this approach looks acceptable, or if there’s anything else you’d like me to adjust?

https://github.com/user-attachments/assets/00c2fb36-9190-4be0-8e85-3ac945611129

durgax04 avatar Nov 09 '25 20:11 durgax04