feat: offline status messaging #1258

DNR800 · 2022-10-04T16:00:23Z

This is the first of 3 PR's to implement the ability to add custom messages to the HeaderBar next to the online/offline status.

I've tried to keep the implementation as close to what is stated on the ticket (link above).

The idea is to provide a hook useOnlineStatusMessage that will be able to effect messaging in the header like..

The work on the HeaderBar itself will be done in the ui repo once this is merged in

DNR800 · 2022-10-04T16:04:35Z

package.json

-        "lint": "d2-style check js && d2-style check text"
+        "lint": "d2-style check js && d2-style check text",
+        "link:all": "yarn workspaces run link",
+        "unlink:all": "yarn workspaces run unlink"


I set these up to make working with the UI library easier - I think if might be worth keeping them in for future work (thought it means having lint scripts in the all services package.json files too)

Means that running yarn link:all in app-runtime and then in ui running yarn link:app-runtime should get them to work together nicely. Happy to remove them though if there is any objection

DNR800 · 2022-10-04T16:17:14Z

services/connected-status/README.md

+
+## Installation
+
+This package is internal to `@dhis2/app-runtime` and generally should not be installed independently.


This is a more a general query than something for the scope of this PR but I notice in a few of the read me files for the services we say..

This package is internal to @dhis2/app-runtime and generally should not be installed independently.

If this is the case do we really want to be publishing the packages for the services themselves? @dhis2/app-service-connected-status will likely get published and others like @dhis2/app-service-alerts are already being published as well. Is there a use case I'm not aware of for doing this?

I agree that one the one hand publishing a package, and on the other hand advising against using it, is confusing. IMO we should keep publishing the packages, and rephrase the sentence to something like this:

This package also comes bundled with @dhis2/app-runtime and generally does not have to be installed independently.

The reason why one would want to install a package like this independently is for non-platform apps. For whatever reason, apps may need to use a different setup and we still want them to be able to use all of the providers/services we offer.

Cool @HendrikThePendric I'll change the wording like you suggest there

As this file has been deleted, I'll look at raising another PR to introduce this change to the other readme files

amcgee

Nice work @DNR800 ! A few high-level comments:

I'm not sure Connect Status is the correct name here - it's not exactly connectivity status, if I'm understanding the feature correctly, because the app can use it to set whatever text it wants in the headerbar
Having a dedicated package, context, and provider for a single text string seems perhaps like it's overkill, is there a reason we shouldn't incorporate this into for example the app-service-config package, which seems to fit more or less logically as it's where we currently perform shell-app communication (although currently unidirectional)? Are there additional State variables that we want to add in the future?
Not specifically related to this PR, and wouldn't need to change anything here as the packages are nicely decoupled, but I worry a bit about putting this status in this location in the headerbar, especially when we need to support i.e. mobile form factors and are allowing arbitrary text to be set. cc @cooper-joe

Happy to sit down and discuss this if it's easier to do in a meeting!

cooper-joe · 2022-10-05T11:25:11Z

Not specifically related to this PR, and wouldn't need to change anything here as the packages are nicely decoupled, but I worry a bit about putting this status in this location in the headerbar, especially when we need to support i.e. mobile form factors and are allowing arbitrary text to be set. cc @cooper-joe

Small screens should use a different pattern where the entire "status" component is shown under the main headerbar area. This makes it easier to run onto multiple lines. I believe this is implemented and working in Dashboard app, for example. (Original specs here, though slightly stale).

DNR800 · 2022-10-05T12:26:21Z

Thanks @amcgee

I had taken naming and implementation ideas that had been suggested in the ticket https://dhis2.atlassian.net/browse/LIBS-230 - I think that the status in the new provider here will eventually supplement the useOnlineStatus online and offline values so eventually it won't just be the string message thats used (at least that was my take away from reading that ticket). useConnectedStatus was put forward as a name so I have gone with that. Happy to discuss better names if you have a better suggestion.
I had some initial guidance from @KaiVandivier on the implementation for this which was to create a new context and hook. I only pulled this out into its own package as it looked like most of the other providers seemed to have their own package and was following that convention. I can look at moving this to app-service-config if you think it's a better fit
Fair point - @cooper-joe I think those designs show the message sitting to the side of the online/offline but I can make sure it drops to the line below if that might be better?

Maybe a quick call would be good to firm up the direction :-)

HendrikThePendric · 2022-10-05T13:20:06Z

Reg. the naming and whether or not this needs to be it's own package, I think it depends on a few things....

Firstly, we currently are (going to be) doing work in a few related areas:

Connectivity status: can the app contact a working DHIS2 Core instance?
Session expiry status: is the current user authenticated at that DHIS2 Core instance?
Connectivity status UI customisation: allow the app to set the status icon+text and add a prefix [current pr]

As discussed yesterday, the connectivity status and session expiry should be combined. And I can imagine that having a dedicated provider for that directly under the DataProvider and the OfflineProvider, and having all of this in a single package would make sense. Obviously all of this is pretty much unrelated to the changes in the current PR, but still worth mentioning because I think we need to consider the bigger picture here.

We also discussed yesterday, that perhaps in the future we don't want to only be able to only control the headerbar connectivity UI elements, but also allow the app to actually "set the app to offline mode". With that in mind it probably makes sense to also colocate the functionality introduced in this PR with the connectivity status and session status functionality. Again a good reason to have everything in a dedicated package.

Having said all of that, even though these things might make sense on a conceptual level, I think there might be some technical challenges here too. It might be easier to integrate all of this into one of our existing packages.

And also, just to be very explicit, I do agree that when looking at the current functionality in isolation, this wouldn't really justify the creating of a new package.

DNR800 · 2022-10-05T14:23:29Z

If there is the possibility that those changes (such connectivity status, session expiry, connectivity status UI customisation) could living in other existing packages then yes having a new provider at this point doesn't make any sense and putting this additional message functionality in one of the other context/hooks makes sense.

One option like mentioned above is that I move the message functionality to @dhis2/app-service-config and change the useConfig hook to supply the onlineStatusMessage value and a function for updating that message? This would probably be the simplest change and at least add the functionality for now.

My only grumble with that is that the message is more like "Application State" than what I would think of as config, as like you say @amcgee, config is kind of uni directional.

Another option might be just to add it to @dhis2/app-service-offline for now as its roughly related to the area where useOnlineStatus is put to use anyway.

services/offline/src/types.ts

KaiVandivier

Looks good to me 🙂

amcgee

Looks good to me!

One thing we might want to consider, though I'm not sure it's critical for this PR - this will cause re-renders of components even if they only ever set the status message, never reading it. I think this is probably a common situation. If we're careful to only use this in places that have low-cost renders it should be OK, but might be worth thinking about.

# [3.6.0](v3.5.0...v3.6.0) (2022-10-19) ### Features * offline status messaging ([#1258](#1258)) ([f22e1f3](f22e1f3))

dhis2-bot · 2022-10-19T17:00:46Z

🎉 This PR is included in version 3.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

KaiVandivier · 2022-10-20T13:03:15Z

Made a jira issue for the optimization follow-up: https://dhis2.atlassian.net/browse/LIBS-369

DNR800 commented Oct 4, 2022

View reviewed changes

DNR800 changed the title ~~Feat: connected status messaging~~ feat: connected status messaging Oct 4, 2022

DNR800 commented Oct 4, 2022

View reviewed changes

amcgee requested changes Oct 5, 2022

View reviewed changes

DNR800 force-pushed the feat/offline-status-messaging branch from 4ff6888 to f3b0cc0 Compare October 7, 2022 15:12

DNR800 changed the title ~~feat: connected status messaging~~ feat: offline status messaging Oct 10, 2022

KaiVandivier reviewed Oct 10, 2022

View reviewed changes

services/offline/src/types.ts Outdated Show resolved Hide resolved

DNR800 added 3 commits October 18, 2022 11:57

feat: add the provider and the hook for connected status

ca91d79

feat: as use online status message hook

3bb185a

fix: remove uneeded type deinitions

781aac6

DNR800 force-pushed the feat/offline-status-messaging branch from f3b0cc0 to 781aac6 Compare October 18, 2022 10:04

KaiVandivier reviewed Oct 18, 2022

View reviewed changes

services/offline/src/types.ts Outdated Show resolved Hide resolved

KaiVandivier reviewed Oct 18, 2022

View reviewed changes

services/offline/src/types.ts Show resolved Hide resolved

KaiVandivier approved these changes Oct 18, 2022

View reviewed changes

fix: small changes following code review and yarn.lock updates

6ad29b6

DNR800 mentioned this pull request Oct 18, 2022

feat: online/offline status messaging dhis2/ui#1163

Merged

3 tasks

amcgee approved these changes Oct 19, 2022

View reviewed changes

DNR800 merged commit f22e1f3 into master Oct 19, 2022

DNR800 deleted the feat/offline-status-messaging branch October 19, 2022 16:46

dhis2-bot added a commit that referenced this pull request Oct 19, 2022

chore(release): cut 3.6.0 [skip ci]

ddcc521

# [3.6.0](v3.5.0...v3.6.0) (2022-10-19) ### Features * offline status messaging ([#1258](#1258)) ([f22e1f3](f22e1f3))

dhis2-bot added the released label Oct 19, 2022

Birkbjo mentioned this pull request Oct 21, 2022

feat(headerbar): integrate offline-status message dhis2/aggregate-data-entry-app#233

Merged

DNR800 mentioned this pull request Oct 31, 2022

docs: update services readme #1281

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: offline status messaging #1258

feat: offline status messaging #1258

DNR800 commented Oct 4, 2022 •

edited

Loading

DNR800 Oct 4, 2022

DNR800 Oct 4, 2022

HendrikThePendric Oct 5, 2022

DNR800 Oct 5, 2022

DNR800 Oct 19, 2022

amcgee left a comment

cooper-joe commented Oct 5, 2022

DNR800 commented Oct 5, 2022 •

edited

Loading

HendrikThePendric commented Oct 5, 2022

DNR800 commented Oct 5, 2022 •

edited

Loading

KaiVandivier left a comment

amcgee left a comment

dhis2-bot commented Oct 19, 2022

KaiVandivier commented Oct 20, 2022


		## Installation

		This package is internal to `@dhis2/app-runtime` and generally should not be installed independently.

feat: offline status messaging #1258

feat: offline status messaging #1258

Conversation

DNR800 commented Oct 4, 2022 • edited Loading

DNR800 Oct 4, 2022

Choose a reason for hiding this comment

DNR800 Oct 4, 2022

Choose a reason for hiding this comment

HendrikThePendric Oct 5, 2022

Choose a reason for hiding this comment

DNR800 Oct 5, 2022

Choose a reason for hiding this comment

DNR800 Oct 19, 2022

Choose a reason for hiding this comment

amcgee left a comment

Choose a reason for hiding this comment

cooper-joe commented Oct 5, 2022

DNR800 commented Oct 5, 2022 • edited Loading

HendrikThePendric commented Oct 5, 2022

DNR800 commented Oct 5, 2022 • edited Loading

KaiVandivier left a comment

Choose a reason for hiding this comment

amcgee left a comment

Choose a reason for hiding this comment

dhis2-bot commented Oct 19, 2022

KaiVandivier commented Oct 20, 2022

DNR800 commented Oct 4, 2022 •

edited

Loading

DNR800 commented Oct 5, 2022 •

edited

Loading

DNR800 commented Oct 5, 2022 •

edited

Loading