Skip to content
This repository has been archived by the owner on Jan 9, 2023. It is now read-only.

feat(network-status): Notify users when they're working offline #2109

Merged
merged 8 commits into from Jun 7, 2020

Conversation

alessioprestileo
Copy link
Contributor

@alessioprestileo alessioprestileo commented May 30, 2020

Fixes #1773.

Changes proposed in this pull request:

  • Implement NetworkStatus component
  • Write tests for NetworkStatusMessage component
  • Render NetworkStatus component in src/HospitalRun.tsx

@jsf-clabot
Copy link

jsf-clabot commented May 30, 2020

CLA assistant check
All committers have signed the CLA.

@gitpod-io
Copy link

gitpod-io bot commented May 30, 2020

@vercel
Copy link

vercel bot commented May 30, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/hospitalrun/hospitalrun-frontend/hvvlu7ucu
✅ Preview: https://hospitalrun-frontend-git-fork-alessioprestileo-master.hospitalrun.now.sh

@matteovivona matteovivona requested review from jackcmeyer, matteovivona and a user May 31, 2020 08:22
@matteovivona matteovivona added the in progress indicates that issue/pull request is currently being worked on label May 31, 2020
@matteovivona matteovivona added this to In progress in Version 2.0 via automation May 31, 2020
@matteovivona matteovivona added this to the v2.0 milestone May 31, 2020
matteovivona
matteovivona previously approved these changes May 31, 2020
@matteovivona matteovivona self-requested a review May 31, 2020 08:24
Copy link
Contributor

@matteovivona matteovivona left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution!
However, there is no way I can trigger the event to work offline. Where do I see working offline?

Edit: ok there is something strange. I tried a second time from the Vercel preview link and I saw the banner only on the first page (I was on patients). Moving on the dashboard, the banner was gone. (I was working with the wifi off). During the third attempt, I always saw the alert correctly. Tested on Chrome.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionality and UI look great. I suggest we'll look into a hook possibility.

src/components/network-status/NetworkStatus.tsx Outdated Show resolved Hide resolved
@alessioprestileo
Copy link
Contributor Author

@tehkapa
I don't know what the problem you faced could be caused by, I tried this link
https://hospitalrun-frontend-git-fork-alessioprestileo-master.hospitalrun.now.sh/labs
on Firefox and Chromium and it seems to work correctly.

@matteovivona
Copy link
Contributor

@tehkapa
I don't know what the problem you faced could be caused by, I tried this link
https://hospitalrun-frontend-git-fork-alessioprestileo-master.hospitalrun.now.sh/labs
on Firefox and Chromium and it seems to work correctly.

Ok, got it. On Safari doesn't work.

@ghost
Copy link

ghost commented Jun 2, 2020

On page reload (like a refresh), the offline message is absent. I think we'd like to notify the user about the status if they are working offline at the page load. This will be nice when user is offline, and then refreshes a page. The status bar would 'stay' there to indicate user is still offline.

@alessioprestileo
Copy link
Contributor Author

On page reload (like a refresh), the offline message is absent. I think we'd like to notify the user about the status if they are working offline at the page load. This will be nice when user is offline, and then refreshes a page. The status bar would 'stay' there to indicate user is still offline.

@kumikokashii I don't understand this comment... If the user is offline, then the page cannot be refreshed, the browser will show an error when trying to refresh the page.
Did you mean something different?

@matteovivona
Copy link
Contributor

On page reload (like a refresh), the offline message is absent. I think we'd like to notify the user about the status if they are working offline at the page load. This will be nice when user is offline, and then refreshes a page. The status bar would 'stay' there to indicate user is still offline.

@kumikokashii I don't understand this comment... If the user is offline, then the page cannot be refreshed, the browser will show an error when trying to refresh the page.
Did you mean something different?

No, when a user reloads a page, HR must continue to work (since it is offline first).

Here's a gif of the problem. https://jmp.sh/EvkscMx
I translate Firefox label: "Lavora non in linea" = "Work offline"

@alessioprestileo
Copy link
Contributor Author

@tehkapa Ok, now I see your point... I didn't know that there was an option in the browser to work offline, I was testing this feature by turning the wi-fi off.
At the moment the NetworkStatusMessage component is only listening to the event dispatched when the network status changes. When it's mounted it assumes the network is online. I can improve it so that when it's mounted it checks the network status using "navigator.onLine" (https://developer.mozilla.org/en-US/docs/Web/API/NavigatorOnLine/onLine).
Is this what you meant @kumikokashii?

@ghost
Copy link

ghost commented Jun 4, 2020

When it's mounted it assumes the network is online. I can improve it so that when it's mounted it checks the network status using "navigator.onLine" (https://developer.mozilla.org/en-US/docs/Web/API/NavigatorOnLine/onLine).

@alessioprestileo That looks like what we are looking for. Thank you 🙂

@alessioprestileo
Copy link
Contributor Author

My commits from today should address all the comments 😃
Sorry for the force-push, not sure why I needed that 🤦

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alessioprestileo Hello! Thank you for the updates. I'm only about 95% that the css style and the test's translation parts are the most optimal, but it's functioning well after a page refresh, and I think it's ready go.

@jackcmeyer jackcmeyer merged commit fa5bcb6 into HospitalRun:master Jun 7, 2020
Version 2.0 automation moved this from In progress to Done Jun 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
in progress indicates that issue/pull request is currently being worked on
Projects
Version 2.0
  
Done
Development

Successfully merging this pull request may close these issues.

Notify users when they're working offline
4 participants