Skip to content
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

Display a notification when a user goes offline #8081

Closed
3 tasks done
aaemnnosttv opened this issue Jan 12, 2024 · 4 comments
Closed
3 tasks done

Display a notification when a user goes offline #8081

aaemnnosttv opened this issue Jan 12, 2024 · 4 comments
Labels
P2 Low priority Type: Enhancement Improvement of an existing feature

Comments

@aaemnnosttv
Copy link
Collaborator

aaemnnosttv commented Jan 12, 2024

Feature Description

This issue is a follow-up to #3820 which introduced the foundational infrastructure. See #8033 (review)


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation Brief

  • Update includes/Core/Util/Health_Checks.php, add new route, say core/site/data/connection-checks. It should return the text "true".
  • In assets/js/hooks/useMonitorInternetConnection.js, update checkInternetConnection callback function to use API from googlesitekit-api instead of apiFetch.
    • Make a request using API.get to the connection-checks, and explicitly pass useCache with value false
    • Remove onlineResponse and it's usage and pass true directly to setIsOnline, since if entered here, request was successful. Otherwise error will be caught and set to false.
  • Include back the useMonitorInternetConnection to the DashboardMainApp and DashboardEntityApp components

Test Coverage

  • Update useMonitorInternetConnection.test.js

QA Brief

Changelog entry

  • Show a notification when the user goes offline.
@aaemnnosttv aaemnnosttv added P2 Low priority Type: Enhancement Improvement of an existing feature labels Jan 12, 2024
@zutigrm zutigrm assigned zutigrm and unassigned zutigrm Jan 16, 2024
@tofumatt tofumatt self-assigned this Jan 17, 2024
@tofumatt
Copy link
Collaborator

tofumatt commented Jan 17, 2024

Rather than make a new route in Site Kit, and increase traffic to the user's site (which also could be, in theory, not on the public internet), is there any reason not to use something like captive.apple.com? It's on the public internet, would mimic the type of response we're looking for, and is already used for this type of feature.

We can ping the WordPress site instead but I figured something online (and something that already exists for this purpose) seems better, plus is less code for us to maintain.

That URL is what I originally proposed in #3820, for what it's worth 😄

@tofumatt tofumatt assigned zutigrm and unassigned tofumatt Jan 17, 2024
@zutigrm
Copy link
Collaborator

zutigrm commented Jan 17, 2024

Hi @tofumatt, the reason why this was switch to backend call in the original implementation as well, was due to the CORS issue with external services (Google and WordPress ones), and for better reliability. But I haven't tried with apple due to the comments, that since this is Google plugin, we should not use Apple service endpoint. Although I did try it now, and that endpoint seems to be the only one without CORS issue. But we don't have guarantees it might not change in the future. So for reliability and better control, I suggested doing the REST endpoint. Techically we could switch to Apple endpoint, if you think above concerns might not be the issue

Let me know what you think

@zutigrm zutigrm assigned tofumatt and unassigned zutigrm Jan 17, 2024
@tofumatt
Copy link
Collaborator

Ah, fair enough.

Using the Apple endpoint I don't really think is an issue, but the CORS issue is an interesting point.

IB ✅

@tofumatt tofumatt removed their assignment Jan 17, 2024
@benbowler benbowler self-assigned this Jan 18, 2024
@benbowler benbowler removed their assignment Jan 19, 2024
@zutigrm zutigrm assigned zutigrm and unassigned zutigrm Jan 22, 2024
@tofumatt tofumatt assigned tofumatt and benbowler and unassigned tofumatt Jan 22, 2024
@benbowler benbowler assigned tofumatt and unassigned benbowler Jan 24, 2024
@tofumatt tofumatt removed their assignment Jan 30, 2024
@mohitwp mohitwp self-assigned this Jan 30, 2024
@mohitwp
Copy link
Collaborator

mohitwp commented Jan 31, 2024

QA Update ✅

  • Tested on dev environment.
  • Verified that the offline notification in Display a notification when a user goes offline #3820 appears when user goes offline.
  • Verified that the when user is online then notification do not display.
  • Verified that if user switch to offline mode then notification display and when user switch back to online then notification do not appear.
Recording.747.mp4

@mohitwp mohitwp removed their assignment Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 Low priority Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

7 participants