-
-
Notifications
You must be signed in to change notification settings - Fork 32
feat: replaced download icon with cloud icon #708
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
Changes from 2 commits
6870ca2
7ed60e9
c444173
3054516
3e0285b
0d72f0d
ece98e2
4e1076c
b860e3f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,13 +26,30 @@ | |
| to="timeline" | ||
| /> | ||
|
|
||
| <nav-button | ||
| data-cy="download-off" | ||
| icon="download-off" | ||
| color="warning" | ||
| v-if="!isEdgeConnected" | ||
| to="edge-connect" | ||
| /> | ||
| <div> | ||
| <v-tooltip bottom> | ||
| <template | ||
| v-if="!isEdgeConnected" | ||
| #activator="{ on, attrs }" | ||
| > | ||
| <div | ||
| v-bind="attrs" | ||
| v-on="on" | ||
| > | ||
| <nav-button | ||
| :id="connectionStatusIcon" | ||
| data-cy="connection-status" | ||
| :icon="connectionStatusIcon" | ||
| :color="connectionIconColor" | ||
| to="edge-connect" | ||
| v-bind="attrs" | ||
| v-on="on" | ||
| /> | ||
| </div> | ||
| </template> | ||
| <span>{{ connectionStatusTooltipText }}</span> | ||
| </v-tooltip> | ||
| </div> | ||
|
|
||
| <!-- Future navbar icons | ||
| <v-text-field | ||
|
|
@@ -174,11 +191,14 @@ export default { | |
| NavButton: () => import('./shared/Button.vue') | ||
| }, | ||
| data: () => ({ | ||
| connectionStatusIcon: 'cloud-off-outline', | ||
| dialog: false, | ||
| drawer: null, // hide drawer on mobile and show on desktop | ||
| on: true, | ||
| connectionStatusTooltipText: 'Disconnected', | ||
| newFavorites: 0, | ||
| newAlerts: 2, | ||
| connectionIconColor: 'warning', | ||
| logo: '../assets/logo5.svg', | ||
| items: [ | ||
| { icon: 'history', text: 'Timeline', link: '/timeline' }, | ||
|
|
@@ -215,6 +235,19 @@ export default { | |
| { icon: 'info', text: 'About Ambianic', link: '/about' } | ||
| ] | ||
| }), | ||
| methods: { | ||
| setConnectionTooltipText () { | ||
| if (this.peerConnectionStatus === 'PEER_DISCONNECTED') { | ||
| this.connectionStatusTooltipText = 'Disconnected' | ||
| this.connectionStatusIcon = 'cloud-off-outline' | ||
| this.connectionIconColor = 'warning' | ||
| } else if (this.peerConnectionStatus === 'PEER_CONNECTING') { | ||
| this.connectionStatusIcon = 'cloud-sync-outline' | ||
| this.connectionIconColor = 'black' | ||
|
||
| this.connectionStatusTooltipText = 'Connecting ...' | ||
| } | ||
| } | ||
| }, | ||
| computed: { | ||
| ...mapState({ | ||
| isEdgeConnected: function (state) { | ||
|
|
@@ -223,16 +256,21 @@ export default { | |
| ) | ||
| const isConnected = state.pnp.peerConnectionStatus === PEER_CONNECTED | ||
| return isConnected | ||
| } | ||
| }, | ||
| peerConnectionStatus: state => state.pnp.peerConnectionStatus | ||
| }) | ||
| }, | ||
| created () { | ||
| if (!this.isEdgeConnected) { | ||
| this.$store.dispatch(PEER_DISCOVER) | ||
| } | ||
|
|
||
| this.setConnectionTooltipText() | ||
| }, | ||
| watch: { | ||
| peerConnectionStatus: function () { | ||
| this.setConnectionTooltipText() | ||
| } | ||
| } | ||
| } | ||
| </script> | ||
|
|
||
| <style lang="scss" scoped> | ||
| </style> | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| <template> | ||
| <div id="ConnectionStatusSnack-ctn"> | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Micro-Learning Topic: Race condition (Detected by phrase)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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't consider this an edge case. A system wide message should be only shown once.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay. Should I proceed with a fix for that in this PR?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, please implement the flag to clear shown messages in this PR. Open a separate issue for system message queuing. |
||
| <v-snackbar v-model="visibility"> | ||
ivelin marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| <p id="snack-message"> | ||
| {{ message }} | ||
| </p> | ||
|
|
||
| <template #action="{ attrs }"> | ||
| <div | ||
| v-bind="attrs" | ||
| id="close-icon" | ||
| @click="handleClose" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @vickywane what about the case when the snackbar closes itself due to timeout and not a click?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand what you are trying to point out. The
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| > | ||
| <Button | ||
| icon="close" | ||
| color="white" | ||
| /> | ||
| </div> | ||
| </template> | ||
| </v-snackbar> | ||
| </div> | ||
| </template> | ||
|
|
||
| <script> | ||
| import { mapState } from 'vuex' | ||
| import AmbButton from '@/components/shared/Button' | ||
|
|
||
| export default { | ||
| name: 'ConnectionStatusSnack', | ||
| data: () => ({ | ||
| visibility: true, | ||
| message: 'Connecting to Ambianic Edge device' | ||
| }), | ||
| components: { | ||
| Button: AmbButton | ||
| }, | ||
| created () { | ||
| this.setConnectionStatusNotification() | ||
| // this.handleClose() | ||
| }, | ||
| computed: { | ||
| ...mapState({ | ||
| peerConnectionStatus: state => state.pnp.peerConnectionStatus | ||
| }) | ||
| }, | ||
| methods: { | ||
| handleClose () { | ||
| this.visibility = false | ||
| }, | ||
| setConnectionStatusNotification () { | ||
| switch (this.peerConnectionStatus) { | ||
| case 'PEER_CONNECTING': | ||
| this.message = 'Connecting to Ambianic Edge device' | ||
| this.visibility = true | ||
| break | ||
| case 'PEER_CONNECTED': | ||
| this.message = 'Connected to Ambianic Edge device' | ||
| this.visibility = true | ||
| break | ||
| case 'PEER_DISCONNECTED': | ||
| this.message = 'Disconnected from Ambianic Edge device' | ||
| this.visibility = true | ||
| break | ||
| default: | ||
| break | ||
| } | ||
| } | ||
| }, | ||
| watch: { | ||
| peerConnectionStatus: function () { | ||
| this.setConnectionStatusNotification() | ||
| } | ||
| } | ||
| } | ||
| </script> | ||
|
|
||
| <style> | ||
| #snack-message { | ||
| padding-top: 5px; | ||
| font-size: .9rem; | ||
| } | ||
| </style> | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| import Vue from 'vue' | ||
| import { mount, createLocalVue } from '@vue/test-utils' | ||
| import Vuetify from 'vuetify' | ||
| import SnackNotification from '@/components/shared/snackNotification' | ||
| import Vuex from 'vuex' | ||
| import { pnpStoreModule } from '@/store/pnp' | ||
| import { PEER_CONNECTED, PEER_CONNECTING, PEER_DISCONNECTED } from '@/store/mutation-types' | ||
|
|
||
| describe('SnackNotification Component', () => { | ||
| let wrapper, store | ||
| const localVue = createLocalVue() | ||
|
|
||
| Vue.use(Vuetify) | ||
| Vue.use(Vuex) | ||
|
|
||
| const vuetify = new Vuetify() | ||
|
|
||
| beforeEach(() => { | ||
| store = new Vuex.Store({ | ||
| modules: { | ||
| pnp: pnpStoreModule | ||
| } | ||
| }) | ||
|
|
||
| wrapper = mount(SnackNotification, { | ||
| localVue, | ||
| vuetify, | ||
| store | ||
| }) | ||
| }) | ||
|
|
||
| afterEach(() => { | ||
| wrapper.destroy() | ||
| }) | ||
|
|
||
| it('It should contain a `p` element and close icon', () => { | ||
| expect(wrapper.find('#snack-message').exists()).toBeTruthy() | ||
| expect(wrapper.find('#close-icon').exists()).toBeTruthy() | ||
| }) | ||
|
|
||
| it('Displayed text changes with `peerConnectionStatus` state', () => { | ||
| const connectingText = 'Connecting to Ambianic Edge device' | ||
| const connectedText = 'Connected to Ambianic Edge device' | ||
| const disconnectedText = 'Disconnected from Ambianic Edge device' | ||
|
|
||
| store.state.pnp.peerConnectionStatus = PEER_CONNECTING | ||
| const connectingComp = mount(SnackNotification, { | ||
| localVue, | ||
| vuetify, | ||
| store | ||
| }) | ||
| expect(connectingComp.find('#snack-message').text()).toBe(connectingText) | ||
|
|
||
| store.state.pnp.peerConnectionStatus = PEER_CONNECTED | ||
| const connectedComp = mount(SnackNotification, { | ||
| localVue, | ||
| vuetify, | ||
| store | ||
| }) | ||
| expect(connectedComp.find('#snack-message').text()).toBe(connectedText) | ||
|
|
||
| store.state.pnp.peerConnectionStatus = PEER_DISCONNECTED | ||
| const disconnectedComp = mount(SnackNotification, { | ||
| localVue, | ||
| vuetify, | ||
| store | ||
| }) | ||
| expect(disconnectedComp.find('#snack-message').text()).toBe(disconnectedText) | ||
| }) | ||
|
|
||
| it('`peerConnectionStatus` status controls SnackNotification visibility', () => { | ||
| store.state.pnp.peerConnectionStatus = PEER_CONNECTED | ||
|
|
||
| const newComponent = mount(SnackNotification, { | ||
| localVue, | ||
| vuetify, | ||
| store | ||
| }) | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @vickywane did you see this comment? |
||
| expect(newComponent.find('#snack-message').exists()).toBeTruthy() | ||
| }) | ||
| }) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vickywane usually Vue component names are not prefixed with
Component. It becomes redundant and noisy as components add up. Let's rename tostatusSnackbarto keep it close to the standard meaning of the material design component names.