-
-
Notifications
You must be signed in to change notification settings - Fork 109
[fix] Resolve redundant node labels and overlays #454 #474
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
3da29d0
9b1f760
9f53968
9994b01
6cf7e1b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| lts/krypton | ||
| lts/krypton | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -253,12 +253,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). | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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` | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,7 +41,7 @@ const NetJSONGraphDefaultConfig = { | |
| clusterRadius: 80, | ||
| clusterSeparation: 20, | ||
| showMetaOnNarrowScreens: false, | ||
| showLabelsAtZoomLevel: 13, | ||
| showMapLabelsAtZoom: 13, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From #454:
Is this being honored and if yes how? |
||
| showGraphLabelsAtZoom: null, | ||
| crs: L.CRS.EPSG3857, | ||
| echartsOption: { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -32,6 +32,12 @@ class NetJSONGraph { | |||||||||
| this.setupGraph(); | ||||||||||
| this.config.onInit.call(this.graph); | ||||||||||
| this.initializeECharts(); | ||||||||||
| if ( | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
| this.config.showMapLabelsAtZoom === undefined && | ||||||||||
| this.config.showLabelsAtZoomLevel !== undefined | ||||||||||
| ) { | ||||||||||
| this.config.showMapLabelsAtZoom = this.config.showLabelsAtZoomLevel; | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
| } | ||||||||||
| // eslint-disable-next-line no-constructor-return | ||||||||||
| return this.graph; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -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]) { | ||||||
|
|
@@ -179,6 +180,9 @@ class NetJSONGraphRender { | |||||
| const baseGraphSeries = {...configs.graphConfig.series}; | ||||||
| const baseGraphLabel = {...(baseGraphSeries.label || {})}; | ||||||
|
|
||||||
| // Added this for label hover issue | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| baseGraphLabel.silent = true; | ||||||
|
|
||||||
| // Shared helper to get current graph zoom level | ||||||
| const getGraphZoom = () => { | ||||||
| try { | ||||||
|
|
@@ -329,7 +333,6 @@ class NetJSONGraphRender { | |||||
| }); | ||||||
|
|
||||||
| nodesData = nodesData.concat(clusters); | ||||||
|
|
||||||
| const series = [ | ||||||
| { | ||||||
| id: "geo-map", | ||||||
|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Treat Using 🔧 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
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Optional low-effort follow-ups to prevent confusion later:
// 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
🧠 Learnings used |
||||||
| itemStyle: { | ||||||
| color: (params) => { | ||||||
| if ( | ||||||
|
|
@@ -602,13 +609,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: { | ||||||
|
|
@@ -620,25 +632,66 @@ class NetJSONGraphRender { | |||||
| }); | ||||||
| } | ||||||
|
|
||||||
| self.echarts.on("mouseover", () => { | ||||||
| // FIX: Removed the variable declaration. We use the one from upper scope. | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| if ( | ||||||
| showMapLabelsAtZoom && | ||||||
| self.leaflet && | ||||||
| self.leaflet.getZoom() >= showMapLabelsAtZoom | ||||||
| ) { | ||||||
| self.echarts.setOption({ | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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({ | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||||||
|
|
@@ -742,9 +795,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, | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ensure files always end with a blank line please