feat: replaced download icon with cloud icon by vickywane · Pull Request #708 · ambianic/ambianic-ui

vickywane · 2021-08-22T14:13:32Z

Purpose

This pull request will fix this issue here.

Approach

New code within this pull request changes the download-off icon to a cloud-off icon to better depict the connection status of the PWA to an edge device

Resolved TODOs within this pull request

Implement color coding of the new icon
Write sufficient unit tests based on the connection status

Merge Checklist

The code change is tested and works locally.
The user and dev documentation is up to date.
There is no commented out code in this PR.
No lint errors (use flake8)
100% test coverage

gitpod-io · 2021-08-22T14:13:35Z

commit-lint · 2021-08-22T14:13:36Z

Features

replaced download icon with cloud icon (6870ca2)

Bug Fixes

corrected toolip texts (7ed60e9)
removed p tag (c444173)
fixed repetitive message bug (3054516)
update package lockfile (0d72f0d)
update package lockfile (ece98e2)
fixed repetitive snackbar messages (4e1076c)
corrected navbar color (b860e3f)

Contributors

vickywane

Commit-Lint commands

You can trigger Commit-Lint actions by commenting on this PR:

@Commit-Lint merge patch will merge dependabot PR on "patch" versions (X.X.Y - Y change)
@Commit-Lint merge minor will merge dependabot PR on "minor" versions (X.Y.Y - Y change)
@Commit-Lint merge major will merge dependabot PR on "major" versions (Y.Y.Y - Y change)
@Commit-Lint merge disable will desactivate merge dependabot PR
@Commit-Lint review will approve dependabot PR
@Commit-Lint stop review will stop approve dependabot PR

vickywane · 2021-08-22T14:43:27Z

@ivelin

I have implemented the basic feature request to replace the download-off icon with the cloud-off icon.

However, I will need some further clarifications to enable me implement the pending items on this pull request.

What's the color code to be used to reflect the connection states?
What are the texts to be used in the tooltips for the connection states?
Should the icon be displayed when a connection is established?

ivelin · 2021-08-22T18:50:07Z

@ivelin

I have implemented the basic feature request to replace the download-off icon with the cloud-off icon.

However, I will need some further clarifications to enable me implement the pending items on this pull request.

What's the color code to be used to reflect the connection states?

What are the texts to be used in the tooltips for the connection states?

Should the icon be displayed when a connection is established?

All good questions. Here is my recommendations:

What's the color code to be used to reflect the connection states?

Regular color codes. Primary color as with the other informative icons on the nav bar.

Optional:

pop-up a temporary text message with a warning color "Edge device connection lost. Reconnecting...".
pop-up a temporary text message with an info color "Edge device connection established."

Side note:
It seems like v-snackbar is the recommended vuetify component for application-wide user messages, while v-alert is the way to go for component scoped alerts. Related resources:

What are the texts to be used in the tooltips for the connection states?

Tooltips can have the same text as the pop-up messages above.

Should the icon be displayed when a connection is established?

I don't think so. Since this is the normal behavior most of the time, it will be unnecessary clutter.

Optional: display cloud-on icon for a few moments and then hide it.

vickywane · 2021-08-23T15:33:49Z

@ivelin I have just implemented the Snackbar component and displayed it in the timeline component.

The cloud icon and Snackbar responds to the PEER_CONNECTED, PEER_DISCONNECTED, PEER_CONNECTING states.
When in PEER_CONNECTING, the cloud icon changes to the cloud-sync icon type to indicate the state.

ivelin

@vickywane nice work! See a couple of cosmetic comments.

src/views/Timeline.vue

vickywane · 2021-08-23T20:31:33Z

@ivelin I just addressed all the last comments; making the snackbar standalone, moving it to the App component and also correcting the displayed texts.

ivelin

@vickywane UI behavior seems correct. I made a comment about consistency of pop-up/snackbar messages with tooltips.

Also, see if you can improve the snackbar UX. Seems to have a bit too much empty space around the text. Also as a best practice we should show an X or a Close button to allow users to manually close it before the timeout period if they choose to.

src/components/NavBar.vue

ivelin · 2021-08-24T16:11:51Z

ivelin

Almost there. A couple of new comments came up.

src/components/shared/snackNotification.vue

ivelin · 2021-08-25T19:05:06Z

src/components/shared/snackNotification.vue

@@ -0,0 +1,82 @@
+<template>
+  <div id="ConnectionStatusSnack-ctn">


@vickywane this component shows its current snackbar message each time a new page is loaded. Try going from Timeline to Settings to About and see what happens. Once a message is shown, some flag should clear and the same message should not be shown again. We probably need a vuex mutation for the flag to take care of race conditions as users switch pages while new system messages are pushed.

We can also think about a message queue. If multiple system messages are pushed, queue them up and show them one at a time until the queue is empty. This could be another PR as it adds a fair bit of new logic to think through.

Micro-Learning Topic: Race condition (Detected by phrase)

What is this? (2min video)

A race condition is a flaw that produces an unexpected result when the timing of actions impact other actions.

Try this challenge in Secure Code Warrior

This is a valid concern and also an advanced use case.

For a quick fix, I think we can limit the Snackbar to only the timeline screen while this edge case is being worked upon in a new PR.

What do you think?

I don't consider this an edge case. A system wide message should be only shown once.
With your suggestion if the user goes away and back to the timeline screen, they will still see the last message.

Okay.

Should I proceed with a fix for that in this PR?

Yes, please implement the flag to clear shown messages in this PR. Open a separate issue for system message queuing.

ivelin

@vickywane still need to fix the repetitieve message issue and open a new one for queuing.

ivelin · 2021-08-27T23:44:52Z

src/components/shared/snackNotification.vue

+        <div
+          v-bind="attrs"
+          id="close-icon"
+          @click="handleClose"


@vickywane what about the case when the snackbar closes itself due to timeout and not a click?

I don't understand what you are trying to point out.

The x icon for closing the snackbar manually, and it can also close after a timeout

The point is that visibility is only switched to false on click, but not if the snackbar closes due to timeout. In effect the problem with showing the latest old message when switching between menu items sill remains.

ivelin · 2021-08-27T23:49:29Z

src/components/shared/snackNotification.vue

+<template>
+  <div id="ConnectionStatusSnack-ctn">
+    <v-snackbar v-model="visibility">
+      <span id="snack-message">


@vickywane
Maybe an easier way to handle this is to condition rendering on a computed visibility attribute (a function) instead of a fixes parameter. When the computed attribute is read by the v-model check, the function can return the current state and set it to false.

computed: visibility: () => { if (this.isMessageNew) { this.isMessageNew = false return true } else { return false }

then in setConnectionStatusNotification before the switch statement:

this.isMessageNew = true

ivelin · 2021-08-27T23:50:39Z

src/components/shared/snackNotification.vue

+    setConnectionStatusNotification () {
+      switch (this.peerConnectionStatus) {
+        case 'PEER_CONNECTING':
+          this.visibility = this.message !== PEER_CONNECTING_NOTIFICATION


@vickywane this repeated check should not be necessary. If the peer state machine fires the same state message multiple times, then something is wrong with the state machine.

This condition was to handle scenarios when a user navigates to a new screen and the setConnectionStatusNotification function is (re)fired.

ivelin · 2021-08-29T15:28:40Z

src/components/shared/snackNotification.vue

+        <div
+          v-bind="attrs"
+          id="close-icon"
+          @click="handleClose"


The point is that visibility is only switched to false on click, but not if the snackbar closes due to timeout. In effect the problem with showing the latest old message when switching between menu items sill remains.

vickywane · 2021-08-29T15:31:28Z

The point is that visibility is only switched to false on click, but not if the snackbar closes due to timeout. In effect the problem with showing the latest old message when switching between menu items sill remains.

Okay, I understand this better. But the timeout is automatically handled by Vuetify. Would you prefer we disable the timeout and make it manual-close only?

vickywane · 2021-08-30T15:11:24Z

@ivelin I have just written a fix to the repetitive bug issue and testing it out, it works as expected.

Taking a closer look into the issue, the Snackbar component was getting recreated on each page navigation, hence, it needed some other way to persist the last shown notification rather than using local state.

I decided to employ the use of localstorage, each time checking that the last stored status is not the same as the one about to be displayed.

On some further notes, the settings page only checks if an edgePeerId is present before it shows that the PWA is connecting. This causes the settings page to lose sync with the Tooltip and Snackbar ( if displayed ). Do we need to make any modifications on the Settings page?

ivelin

@ivelin I have just written a fix to the repetitive bug issue and testing it out, it works as expected.

Glad you are taking the time to carefully think through and test.

Taking a closer look into the issue, the Snackbar component was getting recreated on each page navigation, hence, it needed some other way to persist the last shown notification rather than using local state.

I decided to employ the use of localstorage, each time checking that the last stored status is not the same as the one about to be displayed.

localstorage is more appropriate for persisting state between app sessions. What you need here is a session scoped connectivity state variable. See how we use Vuex store for PnPService and Peer connection status.

On some further notes, the settings page only checks if an edgePeerId is present before it shows that the PWA is connecting. This causes the settings page to lose sync with the Tooltip and Snackbar ( if displayed ). Do we need to make any modifications on the Settings page?

Interesting catch. Can you show exactly the problem with a self documented test case that fails. Once the bug is fixed the test should pass and keep the code from regressing.

ivelin · 2021-08-30T15:26:12Z

src/components/shared/snackNotification.vue

+        switch (this.peerConnectionStatus) {
+          case 'PEER_CONNECTING':
+            this.message = PEER_CONNECTING_NOTIFICATION
+            this.visibility = true


@vickywane
this.visibility = true can be set once before or after the switch. No need to repeat the same code multiple times without clear benefit.

Setting visibility once before the switch will mean the snackbar will be displayed for all notification statuses. Currently, the notification is only being shown for connected, connecting and disconnected statuses.

I have extracted the assignments within each case to a separate method to make follow the DRY convention.

vickywane · 2021-08-30T21:09:37Z

localstorage is more appropriate for persisting state between app sessions. What you need here is a session scoped connectivity state variable. See how we use Vuex store for PnPService and Peer connection status.

I took a second shot at this and I created a new state within the pnp module to store a lastPeerConnectionStatus value. This way, localstorage is not needed.

vickywane · 2021-08-30T21:15:30Z

Interesting catch. Can you show exactly the problem with a self documented test case that fails. Once the bug is fixed the test should pass and keep the code from regressing.

This is not a bug. Its the way the Settings page is built that might need a slight adjustment in a follow-up PR. This line of code within the Settings page has a if condition that evaluates to true when there is an edgePeerId. This should be corrected to use the peerConnectionStatus.

ivelin

My manual testing showing acceptable behavior. See a few cosmetic comments.

ivelin · 2021-08-30T21:45:36Z

src/components/AppFrame.vue

  <div>
    <!-- nav goes here -->
    <NavBar />
+    <ComponentStatusSnack />


@vickywane usually Vue component names are not prefixed with Component. It becomes redundant and noisy as components add up. Let's rename to statusSnackbar to keep it close to the standard meaning of the material design component names.

src/components/shared/snackNotification.vue

ivelin · 2021-08-30T21:53:50Z

tests/unit/components/shared/snackNotification.spec.js

+      vuetify,
+      store
+    })
+


@vickywane how about a couple of test cases to cover the behavior of the status snackbar when the user switches between menus. This will help prevent regression on this known issue.

@vickywane did you see this comment?

ivelin · 2021-08-30T21:58:11Z

On some further notes, the settings page only checks if an edgePeerId is present before it shows that the PWA is connecting. This causes the settings page to lose sync with the Tooltip and Snackbar ( if displayed ). Do we need to make any modifications on the Settings page?

@vickywane to keep this PR moving, let's open a new issue and PR for the settings page. I agree that it should be in sync with the cloud icon and status snackbar.

ivelin · 2021-08-31T21:02:26Z

Implement color coding of the new icon

Write sufficient unit tests based on the connection status

@vickywane Are these tasks/checkboxes left open on purpose?

vickywane · 2021-08-31T21:04:31Z

Implement color coding of the new icon

Write sufficient unit tests based on the connection status

@vickywane Are these tasks/checkboxes left open on purpose?

No.
I will update them now.

vickywane · 2021-08-31T21:15:10Z

@vickywane to keep this PR moving, let's open a new issue and PR for the settings page. I agree that it should be in sync with the cloud icon and status snack bar.

Aright.

I have opened this feature request to follow up on this issue.

ivelin

Please address the pending comments.

ivelin · 2021-09-01T21:54:57Z

src/components/NavBar.vue

+        this.connectionIconColor = 'warning'
+      } else if (this.peerConnectionStatus === 'PEER_CONNECTING') {
+        this.connectionStatusIcon = 'cloud-sync-outline'
+        this.connectionIconColor = 'black'


@vickywane let's use default colors such as warning and info to avoid problems when the OS switches between light and dark themes. We had this issue in the past several times when some contributors pick colors they like for their widgets which do not agree with the colors of widgets elsewhere in the app.

ivelin · 2021-09-01T21:56:15Z

tests/unit/components/shared/snackNotification.spec.js

+      vuetify,
+      store
+    })
+


@vickywane did you see this comment?

# [2.18.0](v2.17.0...v2.18.0) (2021-09-03) ### Bug Fixes * install wizard bugs ([#707](#707)) ([3697b41](3697b41)) ### Features * replace download icon with cloud icon, add status snackbar (PR [#708](#708)), closes [#697](#697) ([501bc6c](501bc6c))

ivelin · 2021-09-03T14:45:57Z

🎉 This PR is included in version 2.18.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

pull-request-size bot added the size/M label Aug 22, 2021

vickywane closed this Aug 22, 2021

vickywane reopened this Aug 22, 2021

vickywane mentioned this pull request Aug 22, 2021

Replace download off icon with wifi on/off icon to indicate connection status in PWA #697

Closed

ivelin marked this pull request as draft August 22, 2021 18:50

pull-request-size bot added size/L and removed size/M labels Aug 23, 2021

vickywane marked this pull request as ready for review August 23, 2021 15:30

vickywane requested a review from ivelin August 23, 2021 15:33

ivelin requested changes Aug 23, 2021

View reviewed changes

vickywane requested a review from ivelin August 23, 2021 20:31

ivelin requested changes Aug 24, 2021

View reviewed changes

src/components/NavBar.vue Outdated Show resolved Hide resolved

feat: replaced download icon with cloud icon

6870ca2

vickywane force-pushed the update/connection-icon branch from 01ccc70 to 6870ca2 Compare August 24, 2021 20:49

fix: corrected toolip texts

7ed60e9

vickywane requested a review from ivelin August 25, 2021 03:39

ivelin requested changes Aug 25, 2021

View reviewed changes

fix: removed p tag

c444173

vickywane requested a review from ivelin August 25, 2021 20:01

ivelin requested changes Aug 25, 2021

View reviewed changes

vickywane added 2 commits August 27, 2021 21:36

fix: fixed repetitive message bug

3054516

update

3e0285b

ivelin requested changes Aug 27, 2021

View reviewed changes

ivelin requested changes Aug 29, 2021

View reviewed changes

vickywane added 2 commits August 29, 2021 16:39

fix: update package lockfile

0d72f0d

fix: update package lockfile

ece98e2

pull-request-size bot added size/XL and removed size/L labels Aug 29, 2021

vickywane requested a review from ivelin August 30, 2021 15:11

ivelin reviewed Aug 30, 2021

View reviewed changes

vickywane requested a review from ivelin August 30, 2021 21:17

ivelin requested changes Aug 30, 2021

View reviewed changes

pull-request-size bot added size/XXL and removed size/XL labels Aug 30, 2021

fix: fixed repetitive snackbar messages

4e1076c

vickywane force-pushed the update/connection-icon branch from c39d0cd to 4e1076c Compare August 31, 2021 13:37

vickywane mentioned this pull request Aug 31, 2021

Rewrite Settings Screen to use peerConnectionStatus vuex state for displayed connection state. #711

Closed

ivelin requested changes Sep 1, 2021

View reviewed changes

fix: corrected navbar color

b860e3f

ivelin merged commit 501bc6c into ambianic:master Sep 3, 2021

ivelin added the released label Sep 3, 2021

github-actions bot locked as resolved and limited conversation to collaborators Sep 4, 2022

		@@ -0,0 +1,82 @@
		<template>
		<div id="ConnectionStatusSnack-ctn">

Uh oh!

Conversation

vickywane commented Aug 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Approach

Resolved TODOs within this pull request

Merge Checklist

Uh oh!

gitpod-io bot commented Aug 22, 2021

Uh oh!

commit-lint bot commented Aug 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Features

Bug Fixes

Contributors

Uh oh!

vickywane commented Aug 22, 2021

Uh oh!

ivelin commented Aug 22, 2021

Uh oh!

vickywane commented Aug 23, 2021

Uh oh!

ivelin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vickywane commented Aug 23, 2021

Uh oh!

ivelin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ivelin commented Aug 24, 2021

Uh oh!

ivelin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

secure-code-warrior-for-github bot Aug 25, 2021

Choose a reason for hiding this comment

Micro-Learning Topic: Race condition (Detected by phrase)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivelin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vickywane commented Aug 29, 2021

Uh oh!

vickywane commented Aug 30, 2021

Uh oh!

ivelin left a comment

Choose a reason for hiding this comment

vickywane commented Aug 22, 2021 •

edited

Loading

commit-lint bot commented Aug 22, 2021 •

edited

Loading

vickywane Aug 30, 2021 •

edited

Loading