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

WebUI: Allows one to view a map of the SNMP location set for a device #5495

Merged
merged 4 commits into from Jan 24, 2017

Conversation

Projects
None yet
5 participants
@InsaneSplash
Contributor

InsaneSplash commented Jan 17, 2017

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

  • Have you signed the Contributors agreement - please do NOT submit a pull request unless you have (signing the agreement in the same pull request is fine). Your commit message for signing the agreement must appear as per the docs.
  • Have you followed our code guidelines?
@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Jan 17, 2017

Auto-Deploy finished, Test PR at http://5495.ci.librenms.org or https://5495.ci.librenms.org

@laf

This comment has been minimized.

Member

laf commented Jan 17, 2017

We translate location to lat/lng, it's probably worth checking we have those values (locations table) and if so then display the link using those coords.

Is it also worth changing 'Map' for a globe icon?

@InsaneSplash

This comment has been minimized.

Contributor

InsaneSplash commented Jan 18, 2017

I did wonder about that but as google maps would be searching for the same location, as you have done to translate it to lat/lat, I thought that it would be less complex to merely use the same information provided. It would also help in situations where the user specified a location that is not that accurate, such as a city rather than an exact street address.

I do have a question though, is the lat/long important if the user is not entering it themselves? Maybe in the future we could give the the user a way to specify the lat/long rather using the OpenStreetMap or GoogleMap API to obtain a more accurate value?

I am more than happy to change the way it works based if there is a lat/longresult from the db rather.

I decided on using the map-pin in the end as the globe is already used in the other proposed PR as a http link to the device and so tried to find something less confusing. Its also the same icon used for Locations in our main menu.

@InsaneSplash

This comment has been minimized.

Contributor

InsaneSplash commented Jan 18, 2017

I guess one could also move the link to the device settings drop down with SSH, Telnet, Web, but its current location makes it more accessible.

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Jan 18, 2017

Auto-Deploy finished, Test PR at http://5495.ci.librenms.org or https://5495.ci.librenms.org

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Jan 19, 2017

Auto-Deploy finished, Test PR at http://5495.ci.librenms.org or https://5495.ci.librenms.org

@scrutinizer-notifier

This comment has been minimized.

scrutinizer-notifier commented Jan 19, 2017

The inspection completed: 660 Issues, 18 Patches

@@ -99,7 +99,7 @@
if (is_array($loc)) {
echo '<tr>
<td>Lat / Lng</td>
<td>['.$loc['lat'].','.$loc['lng'].']</td>
<td>['.$loc['lat'].','.$loc['lng'].'] &nbsp;&nbsp;<a href="http://maps.google.com/?q='.$loc['lat'].'+'.$loc['lng'].'" target="_blank"><div class="pull-right"><button type="button" class="btn btn-success btn-xs"><i class="fa fa-map-marker" style="color:white" aria-hidden="true"></i> Map</button></div></a></td>

This comment has been minimized.

@laf

laf Jan 19, 2017

Member

You can merge the a href + button into one:

http://getbootstrap.com/css/#anchor-element

I'd also update the maps.google.com to be https.

@laf

This comment has been minimized.

Member

laf commented Jan 19, 2017

This pr changes the default behaviour and enables lat/lng lookups by default. I'm 100% ok with this. @librenms/reviewers ? We probably should send a notification out.

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Jan 20, 2017

Auto-Deploy finished, Test PR at http://5495.ci.librenms.org or https://5495.ci.librenms.org

@laf

laf approved these changes Jan 20, 2017

@laf

This comment has been minimized.

Member

laf commented Jan 22, 2017

Tagging to merge in 48 hours - @librenms/reviewers

@spinza

This comment has been minimized.

Contributor

spinza commented Jan 23, 2017

Nevermind figured out it's adding a UI element.

Anyway we have locations as strings such as something [38.123,17.12312]. Are these handled?

@laf

This comment has been minimized.

Member

laf commented Jan 23, 2017

If you see "Lat / Lng" in your overview then yes a map icon will appear with this update.

@laf laf merged commit 998750d into librenms:master Jan 24, 2017

2 checks passed

Auto-Deploy Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@InsaneSplash InsaneSplash deleted the InsaneSplash:google-map-link branch Jan 26, 2017

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