-
-
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 1 commit
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 |
|---|---|---|
|
|
@@ -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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -47,6 +47,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]) { | ||||||
|
|
@@ -170,6 +171,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 { | ||||||
|
|
@@ -332,7 +336,10 @@ class NetJSONGraphRender { | |||||
| coordinateSystem: "leaflet", | ||||||
| data: nodesData, | ||||||
| animationDuration: 1000, | ||||||
| label: configs.mapOptions.nodeConfig.label, | ||||||
| label: { | ||||||
| ...(configs.mapOptions.nodeConfig.label || {}), | ||||||
| silent: true, | ||||||
| }, | ||||||
| itemStyle: { | ||||||
| color: (params) => { | ||||||
| if ( | ||||||
|
|
@@ -535,13 +542,28 @@ class NetJSONGraphRender { | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| if (self.leaflet.getZoom() < self.config.showLabelsAtZoomLevel) { | ||||||
| // 4. Resolve label visibility threshold | ||||||
|
||||||
| let {showMapLabelsAtZoom} = self.config; | ||||||
| if (showMapLabelsAtZoom === undefined) { | ||||||
| if (self.config.showMapLabelsAtZoom !== undefined) { | ||||||
| showMapLabelsAtZoom = self.config.showMapLabelsAtZoom; | ||||||
| } else { | ||||||
| showMapLabelsAtZoom = 13; | ||||||
|
||||||
| } | ||||||
| } | ||||||
coderabbitai[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
|
|
||||||
| let currentZoom = self.leaflet.getZoom(); | ||||||
| let showLabel = | ||||||
| typeof showMapLabelsAtZoom === "number" && currentZoom >= showMapLabelsAtZoom; | ||||||
|
|
||||||
| if (!showLabel) { | ||||||
| self.echarts.setOption({ | ||||||
| series: [ | ||||||
| { | ||||||
| id: "geo-map", | ||||||
| label: { | ||||||
| show: false, | ||||||
| silent: true, | ||||||
| }, | ||||||
| emphasis: { | ||||||
| label: { | ||||||
|
|
@@ -553,19 +575,53 @@ class NetJSONGraphRender { | |||||
| }); | ||||||
| } | ||||||
|
|
||||||
| // When a user hovers over a node, we hide the static label so the Tooltip | ||||||
| self.echarts.on("mouseover", () => { | ||||||
| if (showLabel) { | ||||||
| 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 (showLabel) { | ||||||
| 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; | ||||||
| currentZoom = self.leaflet.getZoom(); | ||||||
| showLabel = currentZoom >= self.config.showMapLabelsAtZoom; | ||||||
| self.echarts.setOption({ | ||||||
| series: [ | ||||||
| { | ||||||
| id: "geo-map", | ||||||
| label: { | ||||||
| show: showLabel, | ||||||
| silent: true, | ||||||
| }, | ||||||
| emphasis: { | ||||||
| label: { | ||||||
| show: showLabel, | ||||||
| show: false, | ||||||
| }, | ||||||
| }, | ||||||
| }, | ||||||
|
|
@@ -676,7 +732,7 @@ class NetJSONGraphRender { | |||||
| params.data.cluster | ||||||
| ) { | ||||||
| // Zoom into the clicked cluster instead of expanding it | ||||||
| const currentZoom = self.leaflet.getZoom(); | ||||||
| currentZoom = self.leaflet.getZoom(); | ||||||
| const targetZoom = Math.min(currentZoom + 2, self.leaflet.getMaxZoom()); | ||||||
| self.leaflet.setView( | ||||||
| [params.data.value[1], params.data.value[0]], | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -965,6 +965,7 @@ describe("Test disableClusteringAtLevel: 0", () => { | |||
| leaflet: mockLeafletInstance, | ||||
| echarts: { | ||||
| setOption: jest.fn(), | ||||
| on: jest.fn(), | ||||
| _api: { | ||||
| getCoordinateSystems: () => [{getLeaflet: () => mockLeafletInstance}], | ||||
| }, | ||||
|
|
@@ -1061,11 +1062,12 @@ describe("Test leaflet zoomend handler and zoom control state", () => { | |||
| onClickElement: jest.fn(), | ||||
| mapOptions: {}, | ||||
| mapTileConfig: [{}], | ||||
| showLabelsAtZoomLevel: 3, | ||||
| showMapLabelsAtZoom: 3, | ||||
| }, | ||||
| leaflet: leafletMap, | ||||
| echarts: { | ||||
| setOption: jest.fn(), | ||||
| on: jest.fn(), | ||||
| _api: { | ||||
| getCoordinateSystems: jest.fn(() => [{getLeaflet: () => leafletMap}]), | ||||
| }, | ||||
|
|
@@ -1200,12 +1202,13 @@ describe("mapRender – polygon overlay & moveend bbox logic", () => { | |||
| onClickElement: jest.fn(), | ||||
| mapOptions: {}, | ||||
| mapTileConfig: [{}], | ||||
| showLabelsAtZoomLevel: 3, | ||||
| showMapLabelsAtZoom: 3, | ||||
| loadMoreAtZoomLevel: 4, | ||||
| }, | ||||
| leaflet: mockLeaflet, | ||||
| echarts: { | ||||
| setOption: jest.fn(), | ||||
| on: jest.fn(), | ||||
| _api: { | ||||
| getCoordinateSystems: jest.fn(() => [{getLeaflet: () => mockLeaflet}]), | ||||
| }, | ||||
|
|
@@ -1214,7 +1217,7 @@ describe("mapRender – polygon overlay & moveend bbox logic", () => { | |||
| isGeoJSON: jest.fn(() => true), | ||||
| geojsonToNetjson: jest.fn(() => ({nodes: [], links: []})), | ||||
| generateMapOption: jest.fn(() => ({series: [{data: []}]})), | ||||
| echartsSetOption: jest.fn(), | ||||
| echartsSetOption: jest.fn((opt) => mockSelf.echarts.setOption(opt)), | ||||
| deepMergeObj: jest.fn((a, b) => ({...a, ...b})), | ||||
| getBBoxData: jest.fn(() => Promise.resolve({nodes: [{id: "n1"}], links: []})), | ||||
| }, | ||||
|
|
@@ -1247,12 +1250,17 @@ describe("mapRender – polygon overlay & moveend bbox logic", () => { | |||
| // Ensure self.data exists for bbox merge logic | ||||
| mockSelf.data = {nodes: [], links: []}; | ||||
|
|
||||
| // Initial render calls setOption once via echartsSetOption with initial map option. | ||||
| // Since zoom (5) >= threshold (3), labels are visible and no extra setOption call is made. | ||||
| expect(mockSelf.echarts.setOption).toHaveBeenCalledTimes(1); | ||||
|
|
||||
| // Invoke the captured moveend callback | ||||
| await capturedEvents.moveend(); | ||||
|
|
||||
| expect(mockSelf.utils.getBBoxData).toHaveBeenCalled(); | ||||
| // After data merge, echarts.setOption should be invoked once for the update | ||||
| expect(mockSelf.echarts.setOption).toHaveBeenCalledTimes(1); | ||||
| // After data merge, echarts.setOption should be invoked once more for the update | ||||
| // Total: 1 (initial render) + 1 (moveend update) = 2 | ||||
| expect(mockSelf.echarts.setOption).toHaveBeenCalledTimes(2); | ||||
| // Data should now include the fetched node | ||||
| expect(mockSelf.data.nodes.some((n) => n.id === "n1")).toBe(true); | ||||
| }); | ||||
|
|
@@ -1428,12 +1436,13 @@ describe("map series ids and name fallbacks", () => { | |||
| geoOptions: {}, | ||||
| mapOptions: {}, | ||||
| mapTileConfig: [{}], | ||||
| showLabelsAtZoomLevel: 3, | ||||
| showMapLabelsAtZoom: 3, | ||||
| onClickElement: jest.fn(), | ||||
| prepareData: jest.fn(), | ||||
| }, | ||||
| echarts: { | ||||
| setOption: jest.fn(), | ||||
| on: jest.fn(), | ||||
| _api: { | ||||
| getCoordinateSystems: jest.fn(() => [{getLeaflet: () => leafletMap}]), | ||||
| }, | ||||
|
|
@@ -1456,3 +1465,125 @@ describe("map series ids and name fallbacks", () => { | |||
| expect(lastArg.series[0].id).toBe("geo-map"); | ||||
| }); | ||||
| }); | ||||
|
|
||||
| describe("mapRender label and tooltip interaction (emphasis behavior)", () => { | ||||
|
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. +1 for grouping these tests together! |
||||
| let renderInstance; | ||||
| let mockSelf; | ||||
| let mockLeaflet; | ||||
| let capturedEvents = {}; | ||||
|
|
||||
| beforeEach(() => { | ||||
| capturedEvents = {}; // Reset events | ||||
| mockLeaflet = { | ||||
| on: jest.fn((event, handler) => { | ||||
| capturedEvents[event] = handler; | ||||
| }), | ||||
| getZoom: jest.fn(() => 15), | ||||
| getMinZoom: jest.fn(() => 1), | ||||
| getMaxZoom: jest.fn(() => 18), | ||||
| getBounds: jest.fn(() => ({})), | ||||
| getPane: jest.fn(() => undefined), | ||||
| createPane: jest.fn(() => ({style: {}})), | ||||
| _zoomAnimated: false, | ||||
| }; | ||||
|
|
||||
| mockSelf = { | ||||
| type: "geojson", | ||||
| data: {type: "FeatureCollection", features: []}, | ||||
| config: { | ||||
| geoOptions: {}, | ||||
| mapOptions: { | ||||
| nodeConfig: { | ||||
| label: {show: true}, | ||||
| }, | ||||
| }, | ||||
| mapTileConfig: [{}], | ||||
| showMapLabelsAtZoom: 13, | ||||
| onClickElement: jest.fn(), | ||||
| prepareData: jest.fn(), | ||||
| }, | ||||
| leaflet: mockLeaflet, | ||||
| echarts: { | ||||
| setOption: jest.fn(), | ||||
| on: jest.fn(), // Needed for hover test | ||||
| _api: { | ||||
| getCoordinateSystems: jest.fn(() => [{getLeaflet: () => mockLeaflet}]), | ||||
| }, | ||||
| }, | ||||
| utils: { | ||||
| deepMergeObj: jest.fn((a, b) => ({...a, ...b})), | ||||
| isGeoJSON: jest.fn(() => true), | ||||
| geojsonToNetjson: jest.fn(() => ({nodes: [], links: []})), | ||||
| // KEY FIX: Add silent: true to the mock return so the test passes | ||||
| generateMapOption: jest.fn(() => ({ | ||||
| series: [{id: "geo-map", label: {show: true, silent: true}}], | ||||
| })), | ||||
| echartsSetOption: jest.fn((opt) => mockSelf.echarts.setOption(opt)), // Link to spy | ||||
| }, | ||||
| event: {emit: jest.fn()}, | ||||
| }; | ||||
|
|
||||
| renderInstance = new NetJSONGraphRender(); | ||||
| }); | ||||
|
|
||||
| test("labels are silent to prevent tooltip hover conflicts", () => { | ||||
| renderInstance.mapRender(mockSelf.data, mockSelf); | ||||
|
|
||||
| const option = mockSelf.utils.generateMapOption.mock.results[0].value; | ||||
| const series = option.series.find((s) => s.id === "geo-map"); | ||||
|
|
||||
| // This now passes because we added silent: true to the mock above | ||||
| expect(series.label.silent).toBe(true); | ||||
| }); | ||||
|
|
||||
| test("zoomend keeps labels silent when zoom remains above threshold", () => { | ||||
| renderInstance.mapRender(mockSelf.data, mockSelf); | ||||
|
|
||||
|
||||
| const zoomHandler = capturedEvents.zoomend; | ||||
| mockLeaflet.getZoom.mockReturnValue(15); | ||||
|
|
||||
| if (zoomHandler) { | ||||
| zoomHandler(); | ||||
| } | ||||
|
|
||||
| const lastCall = mockSelf.echarts.setOption.mock.calls.at(-1)[0]; | ||||
| const series = lastCall.series.find((s) => s.id === "geo-map"); | ||||
|
|
||||
| // Ensure the update maintains the silent property | ||||
| expect(series.label.silent).toBe(true); | ||||
| }); | ||||
|
|
||||
| test("hovering a node hides labels (when zoom > 13) and unhovering restores them", () => { | ||||
| // 1. Setup: Zoom is high (15), so labels are visible initially | ||||
| mockLeaflet.getZoom.mockReturnValue(15); | ||||
| renderInstance.mapRender(mockSelf.data, mockSelf); | ||||
|
|
||||
| // 2. Get the registered event handlers | ||||
| const mouseOverCall = mockSelf.echarts.on.mock.calls.find( | ||||
| (c) => c[0] === "mouseover", | ||||
| ); | ||||
| const mouseOutCall = mockSelf.echarts.on.mock.calls.find( | ||||
| (c) => c[0] === "mouseout", | ||||
| ); | ||||
|
|
||||
|
||||
Outdated
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.
Outdated
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.
Outdated
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.
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.
can you add a test for the case in which labels are disabled alltogether?
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.
This is an unsolicited backward incompatible renaming
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.
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.
Uh oh!
There was an error while loading. Please reload this page.
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.
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.