[Fix] Resolve redundant node labels and overlays #454 #474
Conversation
Test.mp4 |
Dhruv-ub
changed the title
|
Fixed the no-shadow linting error in src/js/netjsongraph.render.js and finalized the parameter rename in src/js/netjsongraph.config.js. Could you please run the checks again? |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
The patch contains changes to the formatting which are polluting the actual changes. If formatting needs to be updated let's open a PR to master with only formatting changes.
src/js/netjsongraph.config.js
Outdated
| clusterSeparation: 20, | ||
| showMetaOnNarrowScreens: false, | ||
| showLabelsAtZoomLevel: 13, | ||
| showMapLabelsAtZoom: 13, |
There was a problem hiding this comment.
This is an unsolicited backward incompatible renaming
There was a problem hiding this comment.
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.
Dhruv-ub
force-pushed
the
issues/454-fix-redundant-labels
branch
from
5648e36 to
a4054d7
Compare
Dhruv-ub
changed the title
Dhruv-ub
force-pushed
the
issues/454-fix-redundant-labels
branch
4 times, most recently
from
f016515 to
93eed68
Compare
Updated mapRender to hide emphasis labels on hover to prevent overlap with overlays. Ensures labels do not overlap with other map elements when hovering. Fixes openwisp#454
Dhruv-ub
force-pushed
the
issues/454-fix-redundant-labels
branch
from
c6c106d to
465e1e7
Compare
|
I'm working in a WSL (Ubuntu) environment and have run the Prettier formatters, but the QA checks are still failing locally on my machine. I've tried to resolve the formatting/linting issues, but I'm stuck on why the checks persist in failing. Could you please check the logs and advise what might be missing?" |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
I need to insist, please only send the bugfix change, do not send any other change, if QA checks fail, we'll look into it. The PR in the current state contains a lot of other changes unrelated to the bugfix which is time consuming for maintaienrs to review.
Dhruv-ub
force-pushed
the
issues/454-fix-redundant-labels
branch
4 times, most recently
from
baac701 to
a685e69
Compare
Dhruv-ub
force-pushed
the
issues/454-fix-redundant-labels
branch
from
a685e69 to
1f239ae
Compare
|
Done please have a look on why ci build failing. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe change addresses redundant node label and overlay visibility in the geo-map component by introducing a configurable zoom threshold for map label display. A new Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches
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: 3
Fix all issues with AI Agents 🤖
In @src/js/netjsongraph.render.js:
- Around line 538-546: The code currently only branches on showMapLabelsAtZoom
=== undefined, so passing showMapLabelsAtZoom: false isn't treated as an
explicit "disable labels" intent; update the resolution logic to explicitly
handle showMapLabelsAtZoom === false as the disabled case (e.g., check if
showMapLabelsAtZoom === false and preserve/flag that state), otherwise run the
existing undefined fallback to self.config.showLabelsAtZoomLevel or 13; ensure
subsequent code that validates numeric zoom treats false as the explicit disable
option rather than an invalid type.
- Line 694: The variable NewZoom uses PascalCase; rename it to camelCase
(newZoom) by changing the declaration "const NewZoom = self.leaflet.getZoom();"
to "const newZoom = self.leaflet.getZoom();" and update every usage of NewZoom
in this module (e.g., any comparisons, assignments, or calls that reference
NewZoom) to newZoom so identifiers remain consistent with JavaScript naming
conventions.
🧹 Nitpick comments (1)
src/js/netjsongraph.render.js (1)
572-572: Consider preserving descriptive variable names.The variable renames (
currentZoom→cZoom,showLabel→sLabel) reduce readability without clear benefit. The original names were more descriptive and self-documenting.🔎 Suggested improvement
- const cZoom = self.leaflet.getZoom(); - const sLabel = cZoom >= self.config.showLabelsAtZoomLevel; + const currentZoom = self.leaflet.getZoom(); + const showLabel = typeof showMapLabelsAtZoom === "number" && currentZoom >= showMapLabelsAtZoom; self.echarts.setOption({ series: [ { id: "geo-map", label: { - show: sLabel, + show: showLabel, },Also applies to: 579-579
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/netjsongraph.render.js
🧰 Additional context used
🪛 GitHub Actions: netjsongraph.js CI BUILD
src/js/netjsongraph.render.js
[warning] 1-1: Code style issues found. Run Prettier with --write to fix.
[error] 1-1: Prettier check failed. Hint: run 'openwisp-qa-format' or 'Prettier --write' to fix code style issues in this file.
🔇 Additional comments (2)
src/js/netjsongraph.render.js (2)
581-585: Good: Emphasis labels remain hidden to prevent overlay overlap.Setting
emphasis.label.showtofalseunconditionally aligns with the PR objective to prevent label overlap with hover overlays. This ensures that when a user hovers over a node, the overlay tooltip appears without competing with the label.
538-610: Theopenwisp-qa-formatscript does not exist in this repository's package.json. Additionally, the code is already properly formatted—Prettier reports no changes needed. The review comment is based on a non-existent issue.Likely an incorrect or invalid review comment.
| // 4. Resolve label visibility threshold | ||
| let {showMapLabelsAtZoom} = self.config; | ||
| if (showMapLabelsAtZoom === undefined) { | ||
| if (self.config.showLabelsAtZoomLevel !== undefined) { | ||
| showMapLabelsAtZoom = self.config.showLabelsAtZoomLevel; | ||
| } else { | ||
| showMapLabelsAtZoom = 13; | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle showMapLabelsAtZoom: false to disable labels entirely.
According to the PR objectives, setting showMapLabelsAtZoom to false should disable labels entirely. The current logic only checks for undefined, so false will pass through and fail the type check at line 550, resulting in labels being hidden (which may be the desired outcome) but not for the right reasons. Make the intent explicit by handling the false case.
🔎 Proposed fix
- // 4. Resolve label visibility threshold
- let {showMapLabelsAtZoom} = self.config;
- if (showMapLabelsAtZoom === undefined) {
- if (self.config.showLabelsAtZoomLevel !== undefined) {
- showMapLabelsAtZoom = self.config.showLabelsAtZoomLevel;
- } else {
- showMapLabelsAtZoom = 13;
- }
- }
+ // 4. Resolve label visibility threshold
+ let {showMapLabelsAtZoom} = self.config;
+ if (showMapLabelsAtZoom === undefined) {
+ if (self.config.showLabelsAtZoomLevel !== undefined) {
+ showMapLabelsAtZoom = self.config.showLabelsAtZoomLevel;
+ } else {
+ showMapLabelsAtZoom = 13;
+ }
+ }
+ // Explicitly handle false to disable labels
+ if (showMapLabelsAtZoom === false) {
+ showMapLabelsAtZoom = Infinity; // Never show labels
+ }📝 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.
| // 4. Resolve label visibility threshold | |
| let {showMapLabelsAtZoom} = self.config; | |
| if (showMapLabelsAtZoom === undefined) { | |
| if (self.config.showLabelsAtZoomLevel !== undefined) { | |
| showMapLabelsAtZoom = self.config.showLabelsAtZoomLevel; | |
| } else { | |
| showMapLabelsAtZoom = 13; | |
| } | |
| } | |
| // 4. Resolve label visibility threshold | |
| let {showMapLabelsAtZoom} = self.config; | |
| if (showMapLabelsAtZoom === undefined) { | |
| if (self.config.showLabelsAtZoomLevel !== undefined) { | |
| showMapLabelsAtZoom = self.config.showLabelsAtZoomLevel; | |
| } else { | |
| showMapLabelsAtZoom = 13; | |
| } | |
| } | |
| // Explicitly handle false to disable labels | |
| if (showMapLabelsAtZoom === false) { | |
| showMapLabelsAtZoom = Infinity; // Never show labels | |
| } |
🤖 Prompt for AI Agents
In @src/js/netjsongraph.render.js around lines 538-546, The code currently only
branches on showMapLabelsAtZoom === undefined, so passing showMapLabelsAtZoom:
false isn't treated as an explicit "disable labels" intent; update the
resolution logic to explicitly handle showMapLabelsAtZoom === false as the
disabled case (e.g., check if showMapLabelsAtZoom === false and preserve/flag
that state), otherwise run the existing undefined fallback to
self.config.showLabelsAtZoomLevel or 13; ensure subsequent code that validates
numeric zoom treats false as the explicit disable option rather than an invalid
type.
| const currentZoom = self.leaflet.getZoom(); | ||
| const showLabel = currentZoom >= self.config.showLabelsAtZoomLevel; | ||
| const cZoom = self.leaflet.getZoom(); | ||
| const sLabel = cZoom >= self.config.showLabelsAtZoomLevel; |
There was a problem hiding this comment.
Critical: Use the computed showMapLabelsAtZoom threshold, not the legacy property.
Line 573 directly references self.config.showLabelsAtZoomLevel, bypassing the threshold resolution logic established at lines 538-546. This breaks the new showMapLabelsAtZoom configuration option and the fallback mechanism. If showLabelsAtZoomLevel is undefined, this will result in NaN comparisons.
🔎 Proposed fix
- const sLabel = cZoom >= self.config.showLabelsAtZoomLevel;
+ const sLabel = typeof showMapLabelsAtZoom === "number" && cZoom >= showMapLabelsAtZoom;Note: You'll need to ensure showMapLabelsAtZoom is accessible in the zoomend handler scope. Consider storing it in self.config or declaring it in a broader scope.
Committable suggestion skipped: line range outside the PR's diff.
| // Zoom into the clicked cluster instead of expanding it | ||
| const currentZoom = self.leaflet.getZoom(); | ||
| const targetZoom = Math.min(currentZoom + 2, self.leaflet.getMaxZoom()); | ||
| const NewZoom = self.leaflet.getZoom(); |
There was a problem hiding this comment.
Use camelCase for the zoom variable.
NewZoom uses PascalCase, which is conventionally reserved for constructors and classes in JavaScript. Use camelCase for consistency with language conventions.
🔎 Proposed fix
- const NewZoom = self.leaflet.getZoom();
- const targetZoom = Math.min(NewZoom + 2, self.leaflet.getMaxZoom());
+ const currentZoom = self.leaflet.getZoom();
+ const targetZoom = Math.min(currentZoom + 2, self.leaflet.getMaxZoom());Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @src/js/netjsongraph.render.js around line 694, The variable NewZoom uses
PascalCase; rename it to camelCase (newZoom) by changing the declaration "const
NewZoom = self.leaflet.getZoom();" to "const newZoom = self.leaflet.getZoom();"
and update every usage of NewZoom in this module (e.g., any comparisons,
assignments, or calls that reference NewZoom) to newZoom so identifiers remain
consistent with JavaScript naming conventions.
Hey, @Dhruv-ub try running |
@stktyagi Thanks for suggestion but I already ran that command, still I am having issue. |
Can you show me what you see after running |
3 participants
Updated
mapRenderto hide emphasis labels on hover to prevent overlap with overlays.Ensures better visibility by preventing label clutter when interacting with map elements.
Fixes #454
Checklist
Reference to Existing Issue
Closes #454.
Description of Changes
mapRenderto hide emphasis labels on hover to prevent overlap with overlays.showLabelsAtZoomLevelis set tofalse, labels will not display at any zoom level.Screenshot