Skip to content

Conversation

@dee077
Copy link
Member

@dee077 dee077 commented Jun 12, 2025

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 #564.

Description of Changes

  • A floorplan button was added below the device table.
  • On click, a new container is created, and the floorplan map is rendered using Netjsongraph.
  • Currently using static sample data from Netjsongraph's examples.
  • Will integrate dynamic data fetching via REST API and render on AJAX success.

Screenshot

Screencast.from.2025-06-13.00-28-12.webm

Copy link
Member

@pandafy pandafy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dee077 we don't need for openwisp/netjsongraph.js#381 to get merged. You can build your branch of netjsongraph.js locally and copy the netjsongraph.min.js in openwisp-monitoring. Please do this in a separate commit.

https://github.com/openwisp/openwisp-monitoring/blob/master/openwisp_monitoring/device/static/monitoring/js/lib/netjsongraph.min.js

While testing, I noticed a bug which changed the width of the dashboard map container. @cestercian is already working on a fix in #335. You can either override the required CSS here temporarily till that PR is merged.

This worked for me

diff --git a/openwisp_monitoring/device/static/monitoring/css/netjsongraph.css b/openwisp_monitoring/device/static/monitoring/css/netjsongraph.css
index 05cd643..c23870d 100755
--- a/openwisp_monitoring/device/static/monitoring/css/netjsongraph.css
+++ b/openwisp_monitoring/device/static/monitoring/css/netjsongraph.css
@@ -9,7 +9,7 @@
   border: 1px solid #ccc;
   color: #6d6357;
   line-height: 20px;
-  max-width: 400px;
+  /* max-width: 400px; */
   min-width: 200px;
   padding: 10px 15px;
 }

On narrower screens, the overlay shows a horizontal scroll bar. This is inconsistent with the handling of map on narrow screens. The horizontal scroll should not appear. The overlay of the floorplan widget, should automatically adjust to the dimensions of the map container.

Screenshot from 2025-06-23 18-37-33

I understand that right now we are focusing on the API changes. So, I am not pointing out changes required in the UI. We aim to develop a similar UI to the mockups.

Comment on lines 164 to 165
class Meta:
model = DeviceLocation
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you extend the Meta class of FloorplanCoordinatesSerializer, then you won't need to re-define model here.

Suggested change
class Meta:
model = DeviceLocation
class Meta(FloorplanCoordinatesSerializer.Meta):

@dee077 dee077 marked this pull request as ready for review June 28, 2025 05:16
Copy link
Member

@pandafy pandafy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dee077 whenever you create a PR which depends on a different PR in another module, please update the requirements.txt file like below so installing openwisp-monitoring using pip would automatically install the correct version of openwisp-controller.

diff --git a/requirements.txt b/requirements.txt
index 6f4b6b0..d1c706f 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -1,4 +1,6 @@
-openwisp-controller @ https://github.com/openwisp/openwisp-controller/tarball/1.2
+openwisp-controller @ https://github.com/dee077/openwisp-controller/tarball/feature/828-api-indoor-map-coordinates
 influxdb~=5.3.2
 django-nested-admin~=4.1.1
 python-dateutil>=2.7.0,<3.0.0

Without this, CI would fail. We didn't encounter this yet since we don't have CI enabled on the gsoc25-map branch yet.

I noticed an issue in the UI
Steps to replicate

  1. Open a floorplan and interact with the floorplan (zoom in/zoom out)
  2. Close the floorplan
  3. Zoom out the geographic map
device-map-2025-07-01_16.29.15.mp4

@dee077 dee077 force-pushed the feature/564-overlay-floorplan-map branch from 72acd27 to c39cc69 Compare July 5, 2025 17:22
@dee077 dee077 force-pushed the feature/564-overlay-floorplan-map branch from 6d717df to 135c683 Compare July 12, 2025 02:46
Comment on lines 256 to 263
map.setView([0,0], 0)
};
// Todo: Explore some better approach if exists
const fullScreen = $('#floorplan-container .leaflet-control-fullscreen-button');
fullScreen.off("click.zoomOnFullscreen"); // avoid duplicates
fullScreen.on("click.zoomOnFullscreen", () => {
map.setZoom(map.getZoom() + 1);
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment explaining what we are trying to achieve here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

device-map-2025-07-22_18.55.46.mp4

The zoom level changes drastically when entering/exiting the fullscreen mode.

@dee077 dee077 force-pushed the feature/564-overlay-floorplan-map branch from 3cc1a7e to ffa29b9 Compare July 22, 2025 14:00
@dee077
Copy link
Member Author

dee077 commented Jul 28, 2025

Closing as #688 is built on top of this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants