-
-
Notifications
You must be signed in to change notification settings - Fork 149
[feature] Setting url fragments and applying to that data on both maps #703
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: gsoc25-map
Are you sure you want to change the base?
Conversation
1ed30c3 to
acec94b
Compare
f570034 to
e533011
Compare
e533011 to
b5cd494
Compare
632f83b to
0fa8b29
Compare
pandafy
left a comment
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.
Amazing work Deepanshu! 🚀 🚀 🚀
This PR is on point!!! Just need to add some comments here and there.
| const rawUrlFragments = window.location.hash.replace(/^#/, ""); | ||
| const fragments = rawUrlFragments.split(";").filter((f) => f.trim() !== ""); |
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.
Explain with a comment what are we doing and why are we doing.
Add a TODO to replace this logic after adding a static re-usable method in NetjsonGraph
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.
Added comments!
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.
@dee077 have we already added a re-usable method in netjsongraph.js? If so, can we use it here?
ef9e7c4 to
0fa8b29
Compare
af11750 to
838bb84
Compare
pandafy
left a comment
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.
@dee077 Good work with adding the buttons to the device page. But, these buttons are redirecting the user to /admin/ page. The end goal is to create a dedicated page for the map.
I'm unsure if we need field label for the button. I'm attaching the screenshot so @nemesifier can confirm quickly.
openwisp_monitoring/device/static/monitoring/js/location-inline.js
Outdated
Show resolved
Hide resolved
b7aa2d6 to
ff2c879
Compare
Updates
|
pandafy
left a comment
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.
The dedicated map page works well. I'm still unsure about adding a model just for adding an admin view.
openwisp_monitoring/device/static/monitoring/js/location-inline.js
Outdated
Show resolved
Hide resolved
Set URL fragments when clicking nodes in geo and indoor maps, and apply state from URL on load. Fixes #562
ff2c879 to
e6d3c2c
Compare
pandafy
left a comment
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 have done a thorough review and it looks good to merge except these small details.
@dee077 the CI is failing due to a selenium test. Can you check why?
| const rawUrlFragments = window.location.hash.replace(/^#/, ""); | ||
| const fragments = rawUrlFragments.split(";").filter((f) => f.trim() !== ""); |
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.
@dee077 have we already added a re-usable method in netjsongraph.js? If so, can we use it here?
| function removeUrlFragment(locationId) { | ||
| if (locationId != null) { | ||
| const id = maps[currentFloor].config.bookmarkableActions.id; | ||
| maps[currentFloor].utils.removeUrlFragment(id); | ||
| } | ||
| } |
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 please help me remember why do we need this?
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.
const rawUrlFragments = window.location.hash.replace(/^#/, "");
const fragments = rawUrlFragments.split(";").filter((f) => f.trim() !== "");
We need the above lines for opening the floorplan overlay div by that time we don't have a netjsongraph instance for the indoor map, so we can't use the method from utils.
removeUrlFragment is used for removing the fragments on closing the overlay div for the indoor map
30ce3fc to
63d61f5
Compare
63d61f5 to
f414c72
Compare
Checklist
Reference to Existing Issue
Closes #562.
Description of Changes
urlFragementsfrom config on both maps to allow fragments to set and apply from netjsongraph.jsfloorplan.jsto parse the params first, and if it exists for the indoor map open the floorplan overlayBlockers