Skip to content

Conversation

dee077
Copy link
Contributor

@dee077 dee077 commented Aug 17, 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 #238 .

Description of Changes

  • Update URL query params on map interactions (zoom, move, bounds change)
  • Add mode in URL to differentiate indoor map and geo-map
  • Open floorplan overlay when in indoor map mode

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

As discussed in our weekly meeting, the result doesn't seem to match the specs described in #238.

Please read again the specs and ask any question if not clear.

@dee077 dee077 force-pushed the feature/238-update-url-on-map-actions branch from 6fa8c24 to 591ed33 Compare September 7, 2025 13:19
@dee077 dee077 marked this pull request as ready for review September 7, 2025 20:53
@dee077 dee077 force-pushed the feature/238-update-url-on-map-actions branch 2 times, most recently from 4c6fdfb to 751d759 Compare September 11, 2025 22:25
@dee077
Copy link
Contributor Author

dee077 commented Sep 11, 2025

Screencast.from.2025-09-12.02-47-39.webm

@dee077 dee077 moved this from In progress to In review in [GSoC25] General Map: Indoor, Mobile, Linkable URLs Sep 12, 2025
@dee077 dee077 force-pushed the feature/238-update-url-on-map-actions branch from 0b32122 to 5bfbddc Compare September 12, 2025 16:11
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.

Good work @Deepanshu! This is going to be a very user feature in the library. 💪🏼

I found a corner case while going back through the history of the browser using the back button. I expected the map to update when the URL fragment updates, but that is not the case here.

I also explored about enabling this feature by default. If ID is not provided in the config, we can generate a hash based on the HTML element. This can have side-effects, so let's think before going ahead in this direction.

Maybe, auto-generating ID is a bad idea. If the hash will change if the user changed the HTML element. This will render old URLs useless.

diff --git a/src/js/netjsongraph.config.js b/src/js/netjsongraph.config.js
index b4fcc67..444d813 100644
--- a/src/js/netjsongraph.config.js
+++ b/src/js/netjsongraph.config.js
@@ -285,7 +285,7 @@ const NetJSONGraphDefaultConfig = {
   ],
   linkCategories: [],
   urlFragments: {
-    show: false,
+    show: true,
     id: null,
   },
 
diff --git a/src/js/netjsongraph.core.js b/src/js/netjsongraph.core.js
index 1d448d1..1758e0b 100644
--- a/src/js/netjsongraph.core.js
+++ b/src/js/netjsongraph.core.js
@@ -57,6 +57,19 @@ class NetJSONGraph {
       console.error("Can't change el again!");
     }
 
+    if (!this.config.urlFragments.id) {
+      // The config does not specify a an ID, we assign one by creating
+      // a hash of the HTML element using FNV-1a algorithm
+      let str = this.el.outerHTML
+      let hash = 0x811c9dc5; // FNV offset basis
+      for (let i = 0; i < str.length; i++) {
+        hash ^= str.charCodeAt(i);
+        hash = (hash * 0x01000193) >>> 0; // FNV prime
+
+      }
+      this.config.urlFragments.id = hash.toString();
+    }
+
     return this.config;
   }

@pandafy
Copy link
Member

pandafy commented Sep 12, 2025

I also explored about enabling this feature by default. If ID is not provided in the config, we can generate a hash based on the HTML element. This can have side-effects, so let's think before going ahead in this direction

Maybe, this is a bad idea. What if the user updates the HTML element? Then, the old URLs will no longer work.

@dee077 dee077 force-pushed the feature/238-update-url-on-map-actions branch from 32eb321 to 0883e25 Compare September 17, 2025 16:19
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.

Good work @dee077! 👏🏼 👏🏼 👏🏼

It works as expected.

We need to add a lot of comments in the code to explain our design decision. Otherwise, we will forget about why we chose to implement the feature this way and then search for the details in issues.

Comment on lines +1258 to +1313
const zoom =
cluster != null ? self.config.disableClusteringAtLevel : self.leaflet.getZoom();
self.leaflet.setView([location.lat, location.lng], zoom);
Copy link
Member

Choose a reason for hiding this comment

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

Didn't we discuss in the last meeting to use a constant zoom on the leaflet map?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is better as the leaflet sets a zoom so that all the nodes are visible on the map initially. Using the initial default zoom set by Leaflet will show the popup along with other locations with this view user can check other locations as well. If we set a custom zoom, the user has to additionally move around on the map.

expect(mockOnClick).toHaveBeenCalledWith("node", node);
});

test("Test applyUrlFragmentState runs only after onReady completes", async () => {
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this test. What are we testing here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We checking the applyUrlFragmentState is called after onRender

@dee077 dee077 force-pushed the feature/238-update-url-on-map-actions branch 2 times, most recently from cc0fb55 to 9c1380f Compare September 21, 2025 23:37
@dee077 dee077 force-pushed the feature/238-update-url-on-map-actions branch 3 times, most recently from 702fba7 to c076184 Compare September 22, 2025 08:46
@dee077 dee077 force-pushed the feature/238-update-url-on-map-actions branch from c076184 to d65b3ac Compare September 22, 2025 09:20
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.

[feature] Make actions bookmarkable
3 participants