[fix] Limit default max zoom level on mapOptions #188 #363

dee077 · 2025-04-16T21:45:00Z

Prevent map from exceeding supported zoom levels of tile providers and added cursor not allowed on hitting min or max zoom level

Fixes #188

Checklist

I have read the OpenWISP Contributing Guidelines.
I have manually tested the changes proposed in this pull request.
I have written new test cases for new code and/or updated existing tests for changes to existing code.
I have updated the documentation.

Reference to Existing Issue

Closes #188.

Description of Changes

Added minZoom:3 and maxZoom:18 to mapOptions
Reduced maxZoom to 18 in mapTileConfig
Added cursor not allowed on hitting max or min zoom level
Updated the same zoom levels in tests

Demo

Screencast.from.2025-04-17.03-11-15.mp4

Prevent map from exceeding supported zoom levels of tile providers and added cursor not allowed on hitting min or max zoom level Fixes openwisp#188

nemesifier

@dee077 thanks for adding the recording! It looks very promising!
I will test and look closer into this asap. 👍 👍

niteshsinha17 · 2025-04-22T13:26:57Z

src/js/netjsongraph.render.js

+      const maxZoom = self.leaflet.getMaxZoom();
+      const minZoom = self.leaflet.getMinZoom();


@dee077 How you figured out that getMaxZoom function is present on leaflet. We could use self.config.

niteshsinha17 · 2025-04-22T13:28:44Z

src/js/netjsongraph.render.js

+      const zoomOutBtn = document.querySelector(".leaflet-control-zoom-out");
+
+      if (currentZoom >= maxZoom) {
+        zoomInBtn.style.cursor = "not-allowed";


instead of cursor not allowed, let's reduce its opacity and set pointer event to none.

We can have a class for this.

dee077 · 2025-04-27T16:42:39Z

Updates

Take zoom values from self.config
Toggle zoom button state via classes instead of directly changing styles
Set floorplan default zoom to 4 (to avoid decimal value conflicts in <= comparison)

nemesifier

@dee077 looks almost ready but please see my comments below.

Can you add 1 browser test for testing maxZoom and minZoom?

nemesifier · 2025-05-02T22:35:02Z

src/js/netjsongraph.render.js

+      const currentZoom = self.leaflet.getZoom();
+      const {maxZoom} = self.config.mapOptions;
+      const {minZoom} = self.config.mapOptions;
+      console.log(minZoom, maxZoom);


Suggested change

console.log(minZoom, maxZoom);

nemesifier · 2025-05-02T22:39:47Z

src/js/netjsongraph.config.js

@@ -238,7 +240,7 @@ const NetJSONGraphDefaultConfig = {
        "http://{s}.tile.openstreetmap.org/{z}/{x}/{y}.png",
      options: {
        minZoom: 3,
-        maxZoom: 32,
+        maxZoom: 18,


Why do we need minZoom and maxZoom to be defined both here and above in mapOptions?

Can we avoid this redundancy? Maybe the mapTileConfig logic can take these settings from mapOptions?
Or would it be possible in each mapTileConfig to override what is in mapOptions? If so, a few code comments would help understanding this more quickly.

I see the minZoom and maxZoom properties of mapTileConfig are already documented in the README, please review the text there to ensure it's up to date.

dee077 · 2025-05-05T12:13:18Z

Updates:

Removed min max zoom from mapTileConfig.
Added selenium test to check the max zoom. The the tile error coming on console is coming late after in between 5s to 10s. That's why added a sleep time of 15s to catch these errors.
Sometimes the test runs slower in ci, should I increase the sleep time to more?

nemesifier

@dee077 @niteshsinha17 this looks good to me now, the only issue is the coverage going down, the browser tests now are excluded from the coverage calculation, do you know if there's a way to include them?

niteshsinha17 · 2025-05-06T19:40:51Z

.github/workflows/ci.yml

@@ -85,11 +85,15 @@ jobs:
          which chromedriver

      - name: Unit tests
-        run: yarn coverage --testPathIgnorePatterns=test/netjsongraph.browser.test.js
-
+        run: npx nyc --silent yarn coverage --testPathIgnorePatterns=test/netjsongraph.browser.test.js


@dee077 @niteshsinha17 this looks good to me now, the only issue is the coverage going down, the browser tests now are excluded from the coverage calculation, do you know if there's a way to include them?

Looks good to me too.

@dee077 @nemesifier I think netjsongraph.browser.test.js is being ignored while running tests and generating coverage. May be need to change this line.

nemesifier

BTW I think this is part of the GSoC project of @cestercian.

I would let @cestercian step in on this. Yash, please review and test.
Can you find a solution for the test coverage?

@dee077 please give write permissions to your repo to @cestercian so he can push to this branch if needed.

In the coming weeks I'll set you up to be able to create branches in the openwisp repositories.

…ged them" This reverts commit 5291568.

Co-Authored-By: Deepanshu Sahu <83969256+dee077@users.noreply.github.com>

cestercian · 2025-05-10T16:58:06Z

Updates:

Removed enabled disabled class addition using js as there's already a class leaflet disabled and leaflet enabled enforced on enabled disabled state
Updated test cases for the same

nemesifier

Help me understand what's going on, the test coverage now doesn't suffer due to the removal of the custom code?

cestercian · 2025-05-11T07:50:21Z

I removed the custom JS that manually toggled zoom button states and instead used Leaflet’s native .leaflet-disabled class (which I discovered while going through the Leaflet docs).

Since there's no custom logic anymore, coverage stays the same because we're just testing config now, not redundant JS.

dee077 · 2025-05-11T08:59:03Z

I have also refactored the code to assign values conditionally instead of using separate if-else blocks in render.js. This reduced the number of uncovered lines in the file, while keeping the tests unchanged. As a result, we observed a slight increase in the coverage percentage.

Previously, when we added custom classes for enabled and disabled states, it introduced additional uncovered lines, which led to a drop in coverage.

But still the selenium tests are not included in the coverage.

nemesifier

I found an issue while testing: clicking on the disabled button seems to move the map, as if the button became transparent with respect to its behavior.

Is that intended? Can it be prevented? Ideally nothing should happen.

max-zoom.webm

removed pointer-event property in src/css

cestercian · 2025-05-14T23:15:01Z

Screen.Recording.2025-05-15.at.4.29.30.AM.mov

pointer-events: none makes the element transparent to pointer interactions — including allowing clicks to pass through to elements underneath. At the time, we actually thought that behavior was beneficial for our use case.

That said, it turned out to cause unintended side effects (like interacting with the map beneath a disabled button), so I’ve removed the property to keep things working as expected. 🙂

nemesifier

Great work @dee077 @cestercian! 👍

I am assuming the mapTileConfig line in the README is still valid also after these changes, right? If not, please update it.

dee077 and others added 2 commits April 17, 2025 03:01

[fix] Limit default max zoom level on mapOptions openwisp#188

b07b788

Prevent map from exceeding supported zoom levels of tile providers and added cursor not allowed on hitting min or max zoom level Fixes openwisp#188

Merge branch 'master' into issues/188-limit-max-zoom-tile-support

63b5974

nemesifier reviewed Apr 18, 2025

View reviewed changes

niteshsinha17 requested changes Apr 22, 2025

View reviewed changes

[fix] Use config zoom values and toggle zoom button classes

7d5a535

nemesifier requested changes May 2, 2025

View reviewed changes

dee077 force-pushed the issues/188-limit-max-zoom-tile-support branch 2 times, most recently from fee11d9 to 6533248 Compare May 5, 2025 12:07

dee077 force-pushed the issues/188-limit-max-zoom-tile-support branch 3 times, most recently from 61e2c40 to eab3ca5 Compare May 5, 2025 12:39

[tests] Added selenium test

186d52d

dee077 force-pushed the issues/188-limit-max-zoom-tile-support branch from eab3ca5 to 186d52d Compare May 5, 2025 12:46

nemesifier reviewed May 6, 2025

View reviewed changes

dee077 force-pushed the issues/188-limit-max-zoom-tile-support branch 3 times, most recently from 57d3d8f to d6413f6 Compare May 6, 2025 11:14

[ci] Add seperate coverage for unit and browser tests and merged them

5291568

dee077 force-pushed the issues/188-limit-max-zoom-tile-support branch from d6413f6 to 5291568 Compare May 6, 2025 11:19

niteshsinha17 reviewed May 6, 2025

View reviewed changes

This comment was marked as duplicate.

Sign in to view

nemesifier reviewed May 9, 2025

View reviewed changes

Merge branch 'master' into issues/188-limit-max-zoom-tile-support

be7d551

cestercian assigned cestercian and unassigned cestercian May 10, 2025

cestercian and others added 2 commits May 10, 2025 21:52

Revert "[ci] Add seperate coverage for unit and browser tests and mer…

1afff21

…ged them" This reverts commit 5291568.

[fix] Remove enable disable class

8e71e02

Co-Authored-By: Deepanshu Sahu <83969256+dee077@users.noreply.github.com>

cestercian requested review from nemesifier and niteshsinha17 May 10, 2025 17:09

nemesifier reviewed May 10, 2025

View reviewed changes

nemesifier assigned dee077 and cestercian May 14, 2025

nemesifier added the enhancement label May 14, 2025

nemesifier added this to OpenWISP Priorities for next releases and [GSoC25] Improve netjsongraph.js resiliency and visualization May 14, 2025

github-project-automation bot moved this to In progress in OpenWISP Priorities for next releases May 14, 2025

nemesifier moved this to In review in [GSoC25] Improve netjsongraph.js resiliency and visualization May 14, 2025

nemesifier reviewed May 14, 2025

View reviewed changes

[change] Pointer event property

fad2cb8

removed pointer-event property in src/css

cestercian requested a review from nemesifier May 14, 2025 23:23

nemesifier approved these changes May 16, 2025

View reviewed changes

nemesifier merged commit 0604185 into openwisp:master May 16, 2025
4 checks passed

github-project-automation bot moved this from In progress to Done in OpenWISP Priorities for next releases May 16, 2025

github-project-automation bot moved this from In review to Done in [GSoC25] Improve netjsongraph.js resiliency and visualization May 16, 2025

This was referenced May 17, 2025

Fix: Restrict Map Zoom Levels to Supported Range #331

Closed

[fix] Reduce max zoom level to prevent unintended behavior due to exc… #260

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix] Limit default max zoom level on mapOptions #188 #363

[fix] Limit default max zoom level on mapOptions #188 #363

dee077 commented Apr 16, 2025

nemesifier left a comment •

edited

Loading

niteshsinha17 Apr 22, 2025

niteshsinha17 Apr 22, 2025

niteshsinha17 Apr 22, 2025 •

edited

Loading

dee077 Apr 22, 2025 •

edited

Loading

dee077 commented Apr 27, 2025

nemesifier left a comment

nemesifier May 2, 2025

nemesifier May 2, 2025

dee077 commented May 5, 2025

nemesifier left a comment

niteshsinha17 May 6, 2025

This comment was marked as duplicate.

nemesifier left a comment •

edited

Loading

cestercian commented May 10, 2025

nemesifier left a comment

cestercian commented May 11, 2025

dee077 commented May 11, 2025 •

edited

Loading

nemesifier left a comment

cestercian commented May 14, 2025

nemesifier left a comment •

edited

Loading

		const maxZoom = self.leaflet.getMaxZoom();
		const minZoom = self.leaflet.getMinZoom();

[fix] Limit default max zoom level on mapOptions #188 #363

[fix] Limit default max zoom level on mapOptions #188 #363

Conversation

dee077 commented Apr 16, 2025

Checklist

Reference to Existing Issue

Description of Changes

Demo

nemesifier left a comment • edited Loading

Choose a reason for hiding this comment

niteshsinha17 Apr 22, 2025

Choose a reason for hiding this comment

niteshsinha17 Apr 22, 2025

Choose a reason for hiding this comment

niteshsinha17 Apr 22, 2025 • edited Loading

Choose a reason for hiding this comment

dee077 Apr 22, 2025 • edited Loading

Choose a reason for hiding this comment

dee077 commented Apr 27, 2025

Updates

nemesifier left a comment

Choose a reason for hiding this comment

nemesifier May 2, 2025

Choose a reason for hiding this comment

nemesifier May 2, 2025

Choose a reason for hiding this comment

dee077 commented May 5, 2025

Updates:

nemesifier left a comment

Choose a reason for hiding this comment

niteshsinha17 May 6, 2025

Choose a reason for hiding this comment

This comment was marked as duplicate.

nemesifier left a comment • edited Loading

Choose a reason for hiding this comment

cestercian commented May 10, 2025

Updates:

nemesifier left a comment

Choose a reason for hiding this comment

cestercian commented May 11, 2025

dee077 commented May 11, 2025 • edited Loading

nemesifier left a comment

Choose a reason for hiding this comment

cestercian commented May 14, 2025

nemesifier left a comment • edited Loading

Choose a reason for hiding this comment

nemesifier left a comment •

edited

Loading

niteshsinha17 Apr 22, 2025 •

edited

Loading

dee077 Apr 22, 2025 •

edited

Loading

nemesifier left a comment •

edited

Loading

dee077 commented May 11, 2025 •

edited

Loading

nemesifier left a comment •

edited

Loading