Features/interface fixes #157

RutgerLubbers · 2018-09-17T12:18:34Z

Hi Howard,

as per previous pull request, this PR with some functionality fixes. We've bundled them together.

The changes are:
Management:

add group
remove group
add map to group
remove map from group
disable map

Weathermap:

fix overlib: show graph next to cursor when hovering
fix clicking on map: go to graphs
fix showing of map?
show groups as tab (design / css)

General:

css consistency (reload, order, etc)

These changes may not be as you would like to see the final release, but it is working now (a bit more :-) ).

Kind regards,

Rutger Lubbers

And build warning fixes

…ce-fixes

…work-weathermap into features/interface-fixes

…ce-fixes

…eatures/interface-fixes

howardjones · 2018-09-17T17:36:42Z

Wow, this is a big one! Thanks! It'll take a little while to go through, but a couple of immediate comments:

What's Q24-Readme?
You're right about the approach not being the intended one, but hey, I haven't really documented that either. This will work. (specifically - the PDO queries you've added are already in MapManager which is intended to be the home for all the non-Cacti-specific parts of the management plugin)

This looks great though, I'll be reading some more later tonight!

howardjones · 2018-09-17T20:05:08Z

Rutger - one other thing that I thought I'd already done, but it turns out I haven't yet. I'm planning to change the license for Weathermap from GPL to MIT. Since this PR is the first major code contribution, I thought I should let you know before anything gets merged, in case that is a problem for you.

pautiina

Some changes are needed, but this can be done later

pautiina · 2018-09-18T06:38:14Z

lib/Weathermap/Integrations/Cacti/WeatherMapCacti10ManagementPlugin.php

@@ -59,13 +59,13 @@ public function handleManagementMainScreen($request, $appObject)
        $locale = $this->manager->application->getLocale();
        print "<h3>This is the React UI below here</h3>";
        print "<h1>INCOMPLETE</h1>";
-        print '<style>@import "cacti-resources/mgmt/main.css";</style>';
+        print '<style>@import "/cacti/plugins/weathermap/cacti-resources/mgmt/main.css";</style>';


It is more correct to do this:
above add global $config;
and replace the string with:
print '<style>@import "' . $config['url_path'] . 'plugins/weathermap/cacti-resources/mgmt/main.css";</style>';

This is the other one I was going to comment on last night, but didn't have the Cacti source handy to check the config item name. Thanks @pautiina.

I really should start testing this for you... but @pautiina is doing a great job and I've too many things to try out too little time.

Should be done now.

pautiina · 2018-09-18T06:39:52Z

lib/Weathermap/Integrations/Cacti/WeatherMapCacti10ManagementPlugin.php

        print "<h3>Testing API</h3>";
        print '<a href="?action=listmaps">Map List</a> - ';
        print '<a href="?action=app_settings">Settings</a> - ';
        print '<a href="?action=listmapfiles">Map Files</a>';
        print "<div id='weathermap-mgmt-root' data-locale='" . $locale . "' data-url='" . $this->makeURL(array("action" => "app_settings")) . "'></div>";
-        print '<script type="text/javascript" src="cacti-resources/mgmt/main.js"></script>';
+        print '<script type="text/javascript" src="/cacti/plugins/weathermap/cacti-resources/mgmt/main.js"></script>';


applying the above global $config
print '<script type="text/javascript" src="' . $config['url_path'] . 'plugins/weathermap/cacti-resources/mgmt/main.js"></script>';

Should be done now.

pautiina · 2018-09-18T06:41:36Z

lib/Weathermap/Integrations/Cacti/WeatherMapCactiManagementPlugin.php

@@ -626,6 +658,23 @@ function handleMapListAdd(
        }
    }

+
+    protected
+    function handleMapsListAdd(


It may be better to stick to the spelling standard to write all in one line:
protected function handleMapsListAdd(

I think that was a mistake. Howie doesn't normally have lines like that. Good spot!

Should be done now.

pautiina · 2018-09-18T06:43:58Z

lib/Weathermap/Integrations/Cacti/WeatherMapCactiUserPlugin.php

        print "<h1>INCOMPLETE</h1>";
-        print '<style src="cacti-resources/user/main.css"></style>';
+        print '<style>@import "cacti-resources/user/main.css"</style>';


also need add global $config and change this line for this:
print '<style>@import "' . $config['url_path'] . 'plugins/weathermap/cacti-resources/user/main.css";</style>';

Should be done now.

pautiina · 2018-09-18T06:46:46Z

lib/Weathermap/Integrations/Cacti/WeatherMapCactiUserPlugin.php

        print "<h1>INCOMPLETE</h1>";
-        print '<style src="cacti-resources/user/main.css"></style>';
+        print '<style>@import "cacti-resources/user/main.css"</style>';
+        print '<script type="text/javascript" src="overlib.js"></script>';


print '<script type="text/javascript" src="' . $config['url_path'] . 'plugins/weathermap/overlib.js"></script>';

Should be done now.

In the cacti world we normally prefix with the $config['url_path'] but if you are using a relative path instead, the first <style> is better for efficiency. Otherwise, the browser has to do multiple steps of retrieving the CSS, importing it into a new style block, parsing that.

The first style might be more efficient. However, there is a (timing?) issue when loading the css.
If we do a 'src="..."' then the CSS does not load (on time), or is not applied.
When we do a '@import ...' then the CSS is applied.

Seems strange, however this fixes some styling issues.

I've run into these kinds of issues too, where a reload fixed styling or Javascript loading. I had assumed it was to do with the way Cacti loads partial pages, so relative URLs aren't always reliable. I haven't looked into it deeply to confirm though.

That was what we experienced too.

The '@import' fixed (works around?) this issue. At least the CSS is now always loaded.

Interesting to know. It should use the current location of the first script as the relative base unless something changes it. So, the import shouldn’t make a difference. Guess if it works, go with it as I doubt any of us have that much free time to waste when there’s a workaround 👍

pautiina · 2018-09-18T06:47:37Z

lib/Weathermap/Integrations/Cacti/WeatherMapCactiUserPlugin.php

-        print '<style src="cacti-resources/user/main.css"></style>';
+        print '<style>@import "cacti-resources/user/main.css"</style>';
+        print '<script type="text/javascript" src="overlib.js"></script>';
+
        print "<div id='weathermap-user-root' data-locale='" . $locale . "' data-url='" . $this->makeURL(array("action" => "settings")) . "'></div>";
        print '<script type="text/javascript" src="cacti-resources/user/main.js"></script>';


Forgot to change this line:
print '<script type="text/javascript" src="' . $config['url_path'] . 'plugins/weathermap/cacti-resources/user/main.js"></script>';

Should be done now.

Again, I would revert to the direct method rather than using @imports

The first style might be more efficient. However, there is a (timing?) issue when loading the css.
If we do a 'src="..."' then the CSS does not load (on time), or is not applied.
When we do a '@import ...' then the CSS is applied.

Seems strange, however this fixes some styling issues.

pautiina · 2018-09-18T06:49:22Z

lib/Weathermap/UI/UIBase.php

@@ -98,7 +100,7 @@ public function validateArgument($type, $value)
        }

        if ($type != "") {
-            throw new WeathermapInternalFail("ValidateArgs saw unknown type");
+            throw new WeathermapInternalFail("ValidateArgs saw unknown type '$type'");


I'm not sure about this line, namely the addition of single quotes

With the single quotes you can see the difference between an empty value, a value with spaces and typos in the message.

However, I'll remove it. :-)

I would have left it as is. As you said, it makes things clear that its an actually value not just a string part of the message.

RutgerLubbers · 2018-09-21T08:16:53Z

Howard, we're fine with the licence change.

Rutger

howardjones

This is looking good to me. My only change requests are:

yarn.lock is supposed to be there for the two react apps
Please don't make random files executable (configs/*.conf for some reason)

pautiina · 2018-10-02T06:36:07Z

I think it can be merge and then change by you requests

pautiina · 2018-10-05T19:46:48Z

@RutgerLubbers can you change?

howardjones · 2018-10-05T20:38:11Z

I merged and then fixed the permissions.

pautiina · 2018-10-05T20:45:55Z

Thith it right

howardjones · 2018-10-05T20:53:33Z

Although it doesn't actually pass the tests. You ran the tests, right, Rutger?

netniV · 2018-10-05T21:24:03Z

@howardjones that's still the ext-snmp error we haven't managed to resolve.

howardjones · 2018-10-05T21:28:07Z

Fixed - the default group ID for a new map should be 1 not 0.

howardjones · 2018-10-05T22:11:02Z

No, this was a unit test failing that previously didn’t. Travis is building cleanly now. https://travis-ci.org/howardjones/network-weathermap/jobs/437817023

…

On Fri, 5 Oct 2018 at 22:24, Mark Brugnoli-Vinten ***@***.***> wrote: @howardjones <https://github.com/howardjones> that's still the ext-snmp error we haven't managed to resolve. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#157 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA3sJRhDhEwbcDrhRUsE171zN5EB2ft_ks5uh830gaJpZM4Wry5n> .

netniV · 2018-10-05T22:16:13Z

I didn't see that one 👍

howardjones · 2018-10-06T06:56:48Z

Anyway - UI is moving forwards... thanks, Rutger & co! :-)

Kay Canoot and others added 19 commits August 9, 2018 13:55

Added all feature changes

b978a6e

Update parameter map.

014c4cd

Updated file permissions.

6212bf6

fixed build warnings

7fd898c

Updated file permissions.

9fc7c51

some styling changes

3d371e6

And build warning fixes

built main.js

5065ae7

Merge branch 'features/vagrant-sans-cacti-user' into features/interfa…

4d89de6

…ce-fixes

Merge branch 'features/vagrant-sans-cacti-user' into features/interfa…

3343b22

…ce-fixes

Merge branch 'features/vagrant-sans-cacti-user' into features/interfa…

6376852

…ce-fixes

Merge branch 'features/vagrant-sans-cacti-user' into features/interfa…

46c3158

…ce-fixes

Merge branch 'features/interface-fixes' of https://github.com/Q24/net…

a248a31

…work-weathermap into features/interface-fixes

Merge branch 'features/vagrant-sans-cacti-user' into features/interfa…

d756fab

…ce-fixes

Trivial fixes (URL and indentation).

5c1700e

Merge remote-tracking branch 'origin/features/interface-fixes' into f…

1fcb7ec

…eatures/interface-fixes

Fixed enable and disable map

5649bb9

Merge remote-tracking branch 'origin/features/interface-fixes' into f…

b27aaa6

…eatures/interface-fixes

Fixed overlib positioning

2bfa8c4

Merged with latest features.

de7630f

pautiina reviewed Sep 18, 2018

View reviewed changes

Rutger Lubbers added 5 commits September 20, 2018 10:05

Removed Q24 readme.

c63afc9

Removed hardcoded cacti path.

7cb76e3

Removed newlines after 'protected' and 'public'.

3d54219

Removed hardcoded cacti path.

9b67f5b

Removed single quotes in exception message.

3d0c5b0

howardjones requested changes Sep 23, 2018

View reviewed changes

howardjones approved these changes Sep 23, 2018

View reviewed changes

This was referenced Oct 2, 2018

Plugin management not working #156

Closed

Weathermap management only shows after the second click on the Weathermap menu option #162

Closed

howardjones merged commit c43952f into howardjones:master Oct 5, 2018

Features/interface fixes #157

Features/interface fixes #157

Conversation

RutgerLubbers commented Sep 17, 2018

howardjones commented Sep 17, 2018

howardjones commented Sep 17, 2018

pautiina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pautiina Sep 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netniV Sep 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netniV Sep 20, 2018 • edited Loading

Choose a reason for hiding this comment

RutgerLubbers commented Sep 21, 2018

howardjones left a comment

Choose a reason for hiding this comment

pautiina commented Oct 2, 2018

pautiina commented Oct 5, 2018

howardjones commented Oct 5, 2018

pautiina commented Oct 5, 2018

howardjones commented Oct 5, 2018

netniV commented Oct 5, 2018

howardjones commented Oct 5, 2018

howardjones commented Oct 5, 2018 via email

netniV commented Oct 5, 2018

howardjones commented Oct 6, 2018

pautiina Sep 18, 2018 •

edited

Loading

netniV Sep 21, 2018 •

edited

Loading

netniV Sep 20, 2018 •

edited

Loading