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

fix(pwa): fix SW update UX #672

Merged
merged 5 commits into from
Oct 11, 2021
Merged

Conversation

KaiVandivier
Copy link
Contributor

@KaiVandivier KaiVandivier commented Oct 5, 2021

Addresses https://jira.dhis2.org/browse/LIBS-237

  • An update prompt is shown instead of reloading automatically on first SW installation
  • When a user clicks 'update' while there are >1 tabs open of the app, there is an additional confirmation dialog that warns about unsaved data being lost when all tabs reload

Includes a substantial refactor of OfflineInterface to become a better, well, offline interface; so update logic can be moved to a React component in the App Adapter that can use things like alerts and DHIS2 UI elements. Update alert logic can be removed from the App-Runtime components now too, which simplifies this update flow and makes it much easier to reason about.

To do:

Known issues:

  • If a user has multiple tabs open, clicks 'Update' on the alert, then cancels the confirmation dialog, the alert closes and the user would have to refresh the page to see the update alert again

To test:

  1. Build pwa and adapter packages, then start PWA app (with --force)
  2. (Assuming you've loaded the PWA app before) you should see the 'Update' prompt normally when the app loads -- go ahead and update.
  3. One client/first installation: In dev tools, make sure there is one client open, unregister the service worker, then reload the page. Verify that the page does not automatically reload when new service worker installs, and an update prompt is shown (Side note: 'recording mode' will not work at this point). Verify that clicking 'Update' reloads the page and recording mode now works. (Side note: refreshing normally works to give the SW control of this tab in this circumstance)
  4. Multiple clients/first installation: This one is weird; bear with me: In dev tools, unregister the SW, refresh the page, unregister again, then duplicate this tab. Both tabs should now install an active service worker, but both are uncontrolled. In either tab, click 'Update' on the alert. Verify a confirmation dialog pops up mentioning reloading and data loss, and that clicking 'Reload' reloads both tabs.
  5. Multiple clients/SW update: Stop and restart the development server. When one tab loads, verify that both tabs show an 'Update' alert. Click 'Update' on one. Verify the 'Reload' confirmation dialog shows, and that both tabs reload when you click 'Reload'

Comment on lines -34 to -35
// Makes sure to take control of available clients when the SW is activated
clientsClaim()
Copy link
Contributor Author

@KaiVandivier KaiVandivier Oct 5, 2021

Choose a reason for hiding this comment

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

Removing this prevents automatic reload on first installation (because of the resulting controllerchange event that triggers a reload). clients.claim() is now only used in response to a user confirming reload after first activation (paired with a reload for all tabs, if a user confirms that too) -- see its implementation below in utils.js and usage in OfflineInterface > init > handleNewSW > onConfirm

Includes OfflineInterface and adapter refactor to use DHIS UI. LIBS-237
@KaiVandivier KaiVandivier changed the title fix(service-worker): fix reload on first install fix(pwa): fix SW update UX Oct 7, 2021
permanent: true,
actions: [
{ label: i18n.t('Update and reload'), onClick: onConfirm },
{ label: i18n.t('Not now'), onClick: () => {} },
Copy link
Contributor

Choose a reason for hiding this comment

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

Will clicking the Not now action close the alert or do we need to implement hide for alerts? If it currently does not work then we can omit this action.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It closes the alert; the 'Not now' was part of Joe's design spec (although I'm not sure an onClick prop is necessary for a no-op action on the alert bar, now that I think about it -- I'll take a look)

Copy link
Contributor Author

@KaiVandivier KaiVandivier Oct 11, 2021

Choose a reason for hiding this comment

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

Looks like onClick is required

@KaiVandivier KaiVandivier merged commit 9eca82e into master Oct 11, 2021
@KaiVandivier KaiVandivier deleted the fix-reload-on-first-install branch October 11, 2021 14:51
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 8.2.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants