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

Borderless fullscreen geographical map, with just the nodes. #8337

Merged
merged 43 commits into from Mar 19, 2018

Conversation

Projects
None yet
6 participants
@dagbdagb
Contributor

dagbdagb commented Mar 8, 2018

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.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926

dagbdagb added some commits Mar 8, 2018

@kkrumm1 kkrumm1 added the WebUI label Mar 8, 2018

@cron410

This comment has been minimized.

Contributor

cron410 commented Mar 8, 2018

+1 ill be testing this out as well

@cron410

This comment has been minimized.

Contributor

cron410 commented Mar 8, 2018

found some whitespaces as follows:

librenms@librenms:~$ ./scripts/github-apply 8337
:13: trailing whitespace.
margin-bottom: 0;
:53: trailing whitespace.
*
:58: trailing whitespace.
*
Checking patch html/css/fullscreenmap.css...
Checking patch html/pages/front/fullscreenmap.php...
Applied patch html/css/fullscreenmap.css cleanly.
Applied patch html/pages/front/fullscreenmap.php cleanly.
warning: 3 lines add whitespace errors.

cron410 and others added some commits Mar 8, 2018

@kkrumm1 kkrumm1 added the Feature label Mar 9, 2018

cron410 and others added some commits Mar 9, 2018

added menu entry
This removes the need for setting it as a front-page. Now it can be accessed from the menu and LibreNMS can be accessed by navigating back.
Create fullscreenmap.inc.php
Notice the line:
$pagetitle[] = 'Fullscreen Map';
Merge pull request #3 from cron410/patch-5
hack to fix some tiles not loading
@laf

This comment has been minimized.

Member

laf commented Mar 9, 2018

This PR has got messy with commits :)

Because you are changing the body css it's causing weird behaviour now for me:

image

See the scroll bar randomly on the right?

@cron410

This comment has been minimized.

Contributor

cron410 commented Mar 9, 2018

@dagbdagb

This comment has been minimized.

Contributor

dagbdagb commented Mar 9, 2018

@laf
sorry about the mess. :-)
First time I am doing anything more advanced than git pull with git. But the mess appears to result in a very nice feature, don't you think?

Do you want us to rediff the final result against a pristine checkout?

@laf

This comment has been minimized.

Member

laf commented Mar 10, 2018

Ok so I don't seem to see the scroll bar anymore after a git pull and a force refresh.

I do see a big gray bar at the top, is that to be expected?

image

@cron410

This comment has been minimized.

Contributor

cron410 commented Mar 10, 2018

@dagbdagb

This comment has been minimized.

Contributor

dagbdagb commented Mar 10, 2018

@laf
That Should Not Happen (tm). Looks like some CSS is not kicking in for you. I'll set up a proper test env.

@dagbdagb dagbdagb changed the title from Alt. frontpage - borderless fullscreen geographical map, with just the nodes. to Borderless fullscreen geographical map, with just the nodes. Mar 10, 2018

@dagbdagb

This comment has been minimized.

Contributor

dagbdagb commented Mar 10, 2018

@laf
I did a fresh install and merged my PR. Map looks great, no gray bar. Is there anything particular about your environment, Neil?

@laf

This comment has been minimized.

Member

laf commented Mar 13, 2018

I've rebased this now so it only shows the relevant changed files.

I'm happy with it now, although should we change the name to 'world map' rather than 'Geographical'?

@dagbdagb

This comment has been minimized.

Contributor

dagbdagb commented Mar 13, 2018

I'd say geographical is better. It does not have to be a world map, unless you zoom it out to cover the world. So, 'geographical' is more generic. I use leaflet config options to zoom in on the area interesting me. (It is the one with the green bikeshed..... :-)

@cron410

This comment has been minimized.

Contributor

cron410 commented Mar 13, 2018

@laf

This comment has been minimized.

Member

laf commented Mar 13, 2018

Geographical doesn't really strike me as obvious to what it is though. I see what you say about the world map but we already call this in the widgets and how much of the world map is shown is down to the user.

It's not really a huge deal but geographical just doesn't seem right.

@scrutinizer-notifier

This comment has been minimized.

scrutinizer-notifier commented Mar 13, 2018

The inspection completed: No new issues

@dagbdagb

This comment has been minimized.

Contributor

dagbdagb commented Mar 13, 2018

I do not have a strong opinion about what we call it. I can live with 'bob'. I fail to see how geographical is wrong, but english is not my first language.

@cron410

This comment has been minimized.

Contributor

cron410 commented Mar 13, 2018

Looks a lot better now, but I get different results at different page zoom levels. Using Chrome on Win10, the page shows the menu bar at the top which is great but the LMS logo diappears. If I zoom in a bit, the logo comes back. Resetting the page back to Zero zoom with Ctrl-0 the logo disappears again as expected. Then if I zoom out, the menu bar disappears completely. I'm willing to find a solution to toggle the menubar CSS field by checkbox if that works in a cleaner way.

I feel like this will affect screens of different resolutions in different ways.

@dagbdagb

This comment has been minimized.

Contributor

dagbdagb commented Mar 13, 2018

@cron410
Unsure if you have issues with CSS or the 'fullscreen detection logic' . You may try to fiddle with the 'fullscreen detection logic' first.
Check out the html here https://pastebin.com/Thfm7pjw. Save it as some file on a webserver and see if the detection logic makes sense in your browser/os combo.

@dagbdagb

This comment has been minimized.

Contributor

dagbdagb commented Mar 13, 2018

Heh. Yeah. I can also see that the zoom-level impacts the fullscreen detection logic. The problem is that there is no good, cross-platform, fullscreen detection logic. And the platform/browser specific methods do not handle the use of F11 for fullscreen toggling. Hence I have to resort to heuristics.

I may look into resetting/setting the zoom-level, but I am not entirely convinced that is a great idea either.

@murrant

This comment has been minimized.

Member

murrant commented Mar 13, 2018

@cron410 you are zooming the the page, not the map. How are you zooming?

@cron410

This comment has been minimized.

Contributor

cron410 commented Mar 13, 2018

@dagbdagb

This comment has been minimized.

Contributor

dagbdagb commented Mar 13, 2018

ctrl-plus/ctrl-minus will exhibit the 'anomaly'.
Be in fullscreen mode with regular zoom.
Hit ctrl-plus until the menubar shows up. The logic/heuristics considers the page to not be fullscreen anymore.

@laf

This comment has been minimized.

Member

laf commented Mar 13, 2018

I feel this is going beyond what's reasonable for this PR now :)

@cron410

This comment has been minimized.

Contributor

cron410 commented Mar 13, 2018

@cron410

This comment has been minimized.

Contributor

cron410 commented Mar 13, 2018

Looks like the issue is only on my tiny laptop screen. Works great on all my coworker's screens and the TV computer. I'm not gonna beat a dead horse.

@cron410

This comment has been minimized.

Contributor

cron410 commented Mar 15, 2018

Are there any other issues preventing a merge?

@laf

laf approved these changes Mar 15, 2018

@kkrumm1

This comment has been minimized.

Member

kkrumm1 commented Mar 16, 2018

Not for me.

@murrant murrant merged commit af39200 into librenms:master Mar 19, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@murrant

This comment has been minimized.

Member

murrant commented Mar 19, 2018

@cron410 nice work :)

inetAnt added a commit to criteo-forks/librenms that referenced this pull request Mar 19, 2018

Borderless fullscreen geographical map, with just the nodes. (librenm…
…s#8337)

* Added a variant of map.php which allows for a borderless fullscreen geographical map, with just the nodes.

* Signed my work.

* Removed a space.

* delete whitespace

* remove whitespace

* hack to fix some tiles not loading

found the fix here:
https://stackoverflow.com/questions/36246815/data-toggle-tab-does-not-download-leaflet-map/36257493#36257493
https://gis.stackexchange.com/questions/224932/problem-with-map-tiles-loading-with-leaflet-and-bootstrap

You might be able to do something more elegant.

* added menu entry

This removes the need for setting it as a front-page. Now it can be accessed from the menu and LibreNMS can be accessed by navigating back.

* Create fullscreenmap.inc.php

Notice the line:
$pagetitle[] = 'Fullscreen Map';

* REmoved the original fullscreenmap.php

* Revert "REmoved the original fullscreenmap.php"

This reverts commit 570953c.

* addded the resize event to html/pages/fullscreenmap.inc.php for proper leaflet rendering

* Removed old file

* Adjusted CSS and javascript.

* Reverting to original javascript.

* change Leaflet container CSS to white background

* Made the Geographical Map not hide the menu when not in fullscreen mode.

* Cleaned up the CSS-file a bit.

* Added a variant of map.php which allows for a borderless fullscreen geographical map, with just the nodes.

* Signed my work.

* Removed a space.

* delete whitespace

* remove whitespace

* hack to fix some tiles not loading

found the fix here:
https://stackoverflow.com/questions/36246815/data-toggle-tab-does-not-download-leaflet-map/36257493#36257493
https://gis.stackexchange.com/questions/224932/problem-with-map-tiles-loading-with-leaflet-and-bootstrap

You might be able to do something more elegant.

* added menu entry

This removes the need for setting it as a front-page. Now it can be accessed from the menu and LibreNMS can be accessed by navigating back.

* Create fullscreenmap.inc.php

Notice the line:
$pagetitle[] = 'Fullscreen Map';

* REmoved the original fullscreenmap.php

* Revert "REmoved the original fullscreenmap.php"

This reverts commit 570953c.

* addded the resize event to html/pages/fullscreenmap.inc.php for proper leaflet rendering

* Removed old file

* Adjusted CSS and javascript.

* Reverting to original javascript.

* change Leaflet container CSS to white background

* Made the Geographical Map not hide the menu when not in fullscreen mode.

* Cleaned up the CSS-file a bit.
@lock

This comment has been minimized.

lock bot commented May 18, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

@lock lock bot locked as resolved and limited conversation to collaborators May 18, 2018

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