Make Weather Page more useful for Network Monitoring [2/2] #6

raleighlittles · 2018-05-24T00:53:58Z

This PR addresses the remaining tasks from the story that were not addressed in the 1st pull request, the main features being:

Now displays number of sites opened/closed/unknown in the sidebar along with each's site basic status (Y, N, or '?' for unknown).
Adjusted the labeling for closure thresholds to distinguish between minimum value thresholds or maximum value thresholds.
Removed node-sass package, which according to npm, had the vulnerable package 'hoek' in its dependency tree.
Now shows the times the site was closed on its "Ok to Open" plot, and hovering over the closed section shows a tool-tip which tells you why the site was closed.
Added a 14-day option.

…e and see why it was closed

…e the site status- not working yyet

…erent approach

…n issue with the asynchronous calback nature of JS

… closed

… in mind for the future

…to load

…n github

…used by not enough CSS padding?

…eded store property

…nnect dots so that theres no gaps in charts, added min/max coloring for thresholds

…ge had a vulnerable package in its dependency tree

markBowman · 2018-05-24T16:43:31Z

The client fails to build for me:

ERROR in /node_modules/css-loader!/node_modules/vue-loader/lib/style-compiler?{"vue":true,"id":"data-v-3a24dfec","scoped":false,"hasInlineConfig":false}!/node_modules/sass-loader/lib/loader.js!/node_modules/vue-loader/lib/selector.js?type=styles&index=0!/src/App.vue
Module build failed: Error: Cannot find module 'node-sass'

…tree?

raleighlittles · 2018-05-24T20:34:01Z

@markBowman My apologies, it worked when I tested it locally (using NPM) and I assumed that meant it would be okay when built with Docker.

I tested it just now with docker and was able to build and run successfully:

[[email protected] client]$ sudo docker ps
CONTAINER ID        IMAGE               COMMAND                  CREATED              STATUS              PORTS                NAMES
265509512d6f        weatherclient       "node /srv/weather..."   About a minute ago   Up About a minute   0.0.0.0:80->80/tcp   serene_lumiere
c71907a58ac0        weatherserver       "go-wrapper run"         About a minute ago   Up About a minute   80/tcp               tender_dijkstra

The weird issue that I'm encountering now is that Docker is still showing that we have a security vulnerability in our dependency tree, even though NPM shows that I'm using the latest version of the package that's that vulnerability.
Running $ npm ls hoekgives me:

[email protected] /home/rlittles/git_projects/weather/client
└─┬ [email protected]
  └── [email protected]

but during the Docker image build step, it still gives the vulnerability error:

npm WARN notice [SECURITY] hoek has 1 moderate vulnerability. Go here for more details: https://nodesecurity.io/advisories?search=hoek&version=2.16.3 - Run `npm i npm@latest -g` to upgrade your npm version, and then `npm audit` to get more info.

You can see that earlier, NPM is reporting that its only the safe version of hoek (5.0.3) is installed, so there appears to be some conflicting info here.

markBowman · 2018-05-24T20:35:47Z

Air Temperature threshold is a minimum, so should be green in the new way of displaying thresholds

markBowman · 2018-05-24T20:40:02Z

TFN open state is showing as unknown but it is open. What causes this?

…shold color to be green, attempting to fix the bad layout issues

…f the viewport

…rrectly resizes at smaller viewport sizes..?

…ertical columns at the right size; removed the unknown counter

raleighlittles · 2018-05-29T19:53:04Z

@Fingel @markBowman To add on to the security vulnerability issue, it looks like we've reached an impasse: the node-sass package (https://www.npmjs.com/package/node-sass) that we use is requiring an older version of the request package, which has the unsafe package in its dependecy tree.

[[email protected] client]$ npm ls hoek
[email protected] /home/rlittles/git_projects/weather/client
├─┬ [email protected]
│ ├─┬ [email protected]
│ │ └── [email protected]  deduped
│ ├── [email protected] 
│ └─┬ [email protected]
│   ├─┬ [email protected]
│   │ ├─┬ [email protected]
│   │ │ └── [email protected]  deduped
│   │ └── [email protected] 
│   └── [email protected]  deduped
└─┬ [email protected]
  └─┬ [email protected]
    └─┬ [email protected]
      ├─┬ [email protected]
      │ └── [email protected]  deduped
      ├── [email protected] 
      └─┬ [email protected]
        └── [email protected]  deduped

Someone opened an issue on the node-sass project a while ago (see: sass/node-sass#2355) but its still open. I tried reverting to an older version of node-sass (4.7.1) but this had an issue building on my system (see: nodejs/node-gyp#1217). Not quite sure what to do here.

Fingel

Looks good! Just a few things to look at.

Fingel · 2018-05-29T22:16:52Z

client/src/App.vue

@@ -1,14 +1,14 @@
 <template>
  <div id="app">
    <section class="section">
-      <div class="container">
+      <div class="container is-fluid is-marginless">


can we remove is-marginless? The page is much easier to read with a bit of a margin.

Fingel · 2018-05-29T22:31:25Z

client/src/components/Site.vue

          </div>
        </div>
        <div class="level-item has-text-centered">
          <div>
            <p class="heading">Pressure mbar</p>
-            <p class="title">{{ datums['Weather Barometric Pressure Value'].data | latestVal }}</p>
+            <p class="title">{{ this.$options.filters.latestResult(datums['Weather Barometric Pressure Value'].data, 'Value') }}</p>


if this is a filter, it should be used as a filter, like this: {{ datums['Weather Barometric Pressure Value'] | latestResult }}

Duly noted. VueJS shows that the two syntaxes are equivalent so I didn't think it mattered which was used; but its fixed in the latest commit.

Fingel · 2018-05-29T22:33:18Z

client/src/components/TimeChart.vue

+              {
+                enabled: true,
+                position: 'left',
+                content: (this.limit_direction === 'min') ? 'minimum' : '',


This label is super distracting, I think it would be a lot better as a legend.

I don't quite think this is supported by the plugin that we use. ChartJS obviously has legends,(https://www.chartjs.org/docs/latest/configuration/legend.html) but I don't see any support for legends in the ChartJS annotation plugin -- the only way I see to mark an annotation is through labels, which is what I'm using now. I can control the location of the label within the plot but thats about it. What I could maybe try doing is only showing the annotation label if you hover over the chart? That could make the chart less busy on first glance but still present the information if needed.

Fingel · 2018-05-29T22:37:56Z

client/src/components/WeatherNav.vue

+            }
+            else {
+              var last_val = resp[resp.length - 1].ValueString;
+              var last_letter = this.$options.filters.statusMsgToLetter(last_val);


Filters are only meant to be used in templates. If this is just a helper method, no need to place it in filters. Use a computed or normal method instead.

Okay, the global filter was removed and code was moved to methods() section.

…unction, changed syntax used for filter in Site template, removed the filters js file since no global filters are used anymore

… offset for labels so that they dont show up off screen

…plain how to use the new click feature?

raleighlittles · 2018-05-30T21:29:21Z

@Fingel @markBowman The changes requested are up, but with regards to the label as a legend, I couldn't quite find a way to display the label as a legend. My approach now was to hide the label, and instead let the user click on the threshold line to toggle the label (minimum or maximum). I originally wanted to use the mouseover/mouseleave events (so you when you hover over the plot it shows up, and when you leave it goes away) but the plugin we used appears to have issues with events (chartjs/chartjs-plugin-annotation#88) so I couldn't quite get that working.

I also added some text below the first plot to tell the user about the functionality -- see:

Let me know what you think.

…ed a bit of spacing between the menu list and the site open/closed counts

Fingel · 2018-05-31T15:36:55Z

Looks good!

markBowman · 2018-05-31T15:46:43Z

The legend to the Sky transparency minimum green line has a value of 0 even though the line is at 25%

markBowman · 2018-05-31T15:49:07Z

OPINION: I would lose the Minimum/Maximum legend from the left side panel because it is not obvious it applies to the plots. You could put it at the very bottom of the main panel under all the plots.

markBowman · 2018-05-31T16:06:13Z

We're getting into the real nitty-gritty of things you weren't asked to do but....

"current values" should be bold (maybe) to make it clear it is a section label
The time period title (e.g. "LAST 24 Hours") is also hard to pick out. Possibly add some space or line separation from the current values. You could also move the Time range picker to here rather than duplicating the information.

markBowman · 2018-05-31T16:08:07Z

If I increase the size of my browser window the graph scale to take advantage but if I shrink it again they do not get smaller so the right side is out of view.

markBowman · 2018-05-31T16:09:19Z

Sorry, didn't mean to hit close.

The green and yellow widgets with number of open and closed sites are clickable buttons with no obvious behavior.

markBowman

Looking really good. Impressive work.

…lue in label since it was incorrect for sky transparency threshold; changed X/Y open/closed buttons to tags so that they cant be clicked; increased size of current values heading and timerange setting heading

raleighlittles · 2018-05-31T20:16:47Z

@markBowman @Fingel Thanks for the feedback. The latest commit has the following changes.

Increased size of "Current Values" heading
Increased size of "Last " heading
Moved legend from sidebar to underneath all plots
Changed buttons to tags so that they're not clickable but still look the same
Removed the minimum/maximum value display from the plot annotation -- I'm not sure why the values weren't working for the Sky Transparency closure, since they worked fine for all the other plots. This wasn't defined in the scope of the story anyways, and I don't think being able to tell the exact value of the threshold is a huge deal, as you can easily discern what the threshold is with relatively good accuracy (i.e. do we really need to tell if the threshold is at 25 and not 22.5, etc.)

Regarding the issue about page resizing, you have to refresh the page if you change the size of the window in the x direction -- the existing version of the LCO Weather page has similar behaviour.

Below are screenshots of the changes.

wrosing · 2018-05-31T23:38:35Z

MUST we continue to call Sky Brightness that instead of Sky Darkness?

…

On Thu, May 31, 2018 at 8:16 PM, Raleigh Littles ***@***.***> wrote: @markBowman <https://github.com/markBowman> @Fingel <https://github.com/Fingel> Thanks for the feedback. The latest commit has the following changes. - Increased size of "Current Values" heading - Increased size of "Last " heading - Moved legend from sidebar to underneath all plots - Changed buttons to tags <https://bulma.io/documentation/elements/tag/> so that they're not clickable but still look the same - Removed the minimum/maximum value display from the plot annotation -- I'm not sure why the values weren't working for the Sky Transparency closure, since they worked fine for all the other plots. This wasn't defined in the scope of the story anyways, and I don't think being able to tell the *exact* value of the threshold is a huge deal, as you can easily discern what the threshold is with relatively good accuracy (i.e. do we really need to tell if the threshold is at 25 and not 22.5, etc.) Regarding the issue about page resizing, you have to refresh the page if you change the size of the window in the x direction -- the existing version of the LCO Weather page has similar behaviour. Below are screenshots of the changes. [image: screenshot from 2018-05-31 13-13-01] <https://user-images.githubusercontent.com/2137201/40805840-71e39f24-64d4-11e8-995f-ccec3e6fb5ee.png> [image: screenshot from 2018-05-31 13-13-20] <https://user-images.githubusercontent.com/2137201/40805841-7200987c-64d4-11e8-8aec-0f89e49ceaa7.png> — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#6 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKedsAO9Hr53DKvWevn0fKTfOIbl6S4pks5t4E-xgaJpZM4ULZla> .

-- Clear skies, Wayne Rosing Founder 805 708 6901 cell 805 880 1603 office Please note [email protected] is our preferred address.

Raleigh Littles added 18 commits May 18, 2018 15:21

tooltips now work -- meaning you can hover over a section marked clos…

b395282

…e and see why it was closed

Small visual changes, preparing to add the status feature to sidebar

8876f00

Early rough draft of trying to use the existing Store pattern to stor…

2b08cbb

…e the site status- not working yyet

Still having a bit of trouble with the Vue store, going to try a diff…

0467ac3

…erent approach

The sidebar now properly displays the status of the site. There was a…

f562113

…n issue with the asynchronous calback nature of JS

Now correctly displays how many sites are open and how many sites are…

c639200

… closed

Added the 28 day option as suggested

866e728

Commenting out some console log statements, added a few TODOs to keep…

9756407

… in mind for the future

Changed 28 day option to 14 day option, 28 day option takes too long …

5fe0c4f

…to load

Some more refactoring, removing commented code

7564a31

Removed package_lock.json file that was causing vulernability error o…

fb14f97

…n github

Removed unused store methods

bbd069b

Fixed issue where Night icon wasnt showing up in the blue box, was ca…

57479b1

…used by not enough CSS padding?

Removed commented code, fixed some method documentation, removed unne…

3e9b743

…eded store property

Comitting changes from merge conflict, as I'm being instructed to by Git

82c3699

Fixed merge conflicts from master, added property to let timelines co…

dc6cf3e

…nnect dots so that theres no gaps in charts, added min/max coloring for thresholds

removed node-sass dependency from package.json file, since that packa…

43c74e9

…ge had a vulnerable package in its dependency tree

removed commented code that i missed earlier

1b16ca7

raleighlittles requested review from Fingel and markBowman May 24, 2018 00:53

Fixed issue with hoek dependency -- no longer shows up in dependency …

121c888

…tree?

Raleigh Littles added 5 commits May 24, 2018 14:38

Trying again to fix the security issues, changed the temperature thre…

4c74d23

…shold color to be green, attempting to fix the bad layout issues

Committing package_lock.json to fix the security issues

cdb7b13

Added is-fluid to container so that it would take up the full space o…

a256442

…f the viewport

looks like the CSS is mostly fixed, overlapping elements are gone, co…

424ee2e

…rrectly resizes at smaller viewport sizes..?

More fiddling with the CSS, trying to see if i can get to switch to v…

8b9e07c

…ertical columns at the right size; removed the unknown counter

Removed inaccurate comment

42264ed

Fingel suggested changes May 29, 2018

View reviewed changes

Raleigh Littles added 3 commits May 29, 2018 16:06

Removed is-marginless, changed global filter to just be a methods() f…

5397f8e

…unction, changed syntax used for filter in Site template, removed the filters js file since no global filters are used anymore

Added feature to only show labels if they're clicked on, added proper…

612e47b

… offset for labels so that they dont show up off screen

Added actual threshold value to min/max label, added short text to ex…

f73b418

…plain how to use the new click feature?

Added the min/max legend in the sidepanel, took out the Tip text, add…

6e2fefb

…ed a bit of spacing between the menu list and the site open/closed counts

Fingel approved these changes May 31, 2018

View reviewed changes

markBowman closed this May 31, 2018

markBowman reopened this May 31, 2018

markBowman approved these changes May 31, 2018

View reviewed changes

Moved legend from sidebar to underneath all plots; removed min/max va…

c165029

…lue in label since it was incorrect for sky transparency threshold; changed X/Y open/closed buttons to tags so that they cant be clicked; increased size of current values heading and timerange setting heading

raleighlittles merged commit d42a5e7 into master May 31, 2018

irasnyd deleted the 156576199-visualization-improvements-2 branch May 2, 2019 23:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Weather Page more useful for Network Monitoring [2/2] #6

Make Weather Page more useful for Network Monitoring [2/2] #6

raleighlittles commented May 24, 2018

markBowman commented May 24, 2018

raleighlittles commented May 24, 2018

markBowman commented May 24, 2018

markBowman commented May 24, 2018

raleighlittles commented May 29, 2018

Fingel left a comment

Fingel May 29, 2018

Fingel May 29, 2018

raleighlittles May 29, 2018

Fingel May 29, 2018

raleighlittles May 29, 2018 •

edited

Loading

Fingel May 29, 2018

raleighlittles May 29, 2018 •

edited

Loading

raleighlittles commented May 30, 2018

Fingel commented May 31, 2018

markBowman commented May 31, 2018

markBowman commented May 31, 2018

markBowman commented May 31, 2018

markBowman commented May 31, 2018

markBowman commented May 31, 2018

markBowman left a comment

raleighlittles commented May 31, 2018

wrosing commented May 31, 2018 via email

Make Weather Page more useful for Network Monitoring [2/2] #6

Make Weather Page more useful for Network Monitoring [2/2] #6

Conversation

raleighlittles commented May 24, 2018

markBowman commented May 24, 2018

raleighlittles commented May 24, 2018

markBowman commented May 24, 2018

markBowman commented May 24, 2018

raleighlittles commented May 29, 2018

Fingel left a comment

Choose a reason for hiding this comment

Fingel May 29, 2018

Choose a reason for hiding this comment

Fingel May 29, 2018

Choose a reason for hiding this comment

raleighlittles May 29, 2018

Choose a reason for hiding this comment

Fingel May 29, 2018

Choose a reason for hiding this comment

raleighlittles May 29, 2018 • edited Loading

Choose a reason for hiding this comment

Fingel May 29, 2018

Choose a reason for hiding this comment

raleighlittles May 29, 2018 • edited Loading

Choose a reason for hiding this comment

raleighlittles commented May 30, 2018

Fingel commented May 31, 2018

markBowman commented May 31, 2018

markBowman commented May 31, 2018

markBowman commented May 31, 2018

markBowman commented May 31, 2018

markBowman commented May 31, 2018

markBowman left a comment

Choose a reason for hiding this comment

raleighlittles commented May 31, 2018

wrosing commented May 31, 2018 via email

raleighlittles May 29, 2018 •

edited

Loading

raleighlittles May 29, 2018 •

edited

Loading