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 "in progress" and "error" states of UpdateStatus component #2794

Merged
merged 3 commits into from
Jan 9, 2023

Conversation

tillprochaska
Copy link
Contributor

@tillprochaska tillprochaska commented Jan 3, 2023

This fixes the status indicators in network diagrams and timelines. See below for videos showing the behavior.

  • When changes are currently being saved, it displays "Saving"
  • When an error has occurred while saving, it displays "Error saving"
  • When the browser is offline, it displays "Offline"

I think ideally, we should also implement further measures to prevent users from losing unsaved work:

  1. Enable a read-only mode when the users browser is offline and show a more prominent, potentially blocking message. This may be slightly more complex to implement, and I’d suggest we look into this separately from this PR.

  2. When a user tries to close the page or to navigate away while there are still unsaved changes, display a warning and ask the user to confirm. We had this behavior before, but it’s currently not possible to implement it, because React Router v6 has removed a relevant feature in v6. There is an active draft PR React Router by the projects maintainer, so it will probably be added back soon. I’d suggest we wait for that rather than working/hacking around it.

Closes #2793.


Error while saving changes:

default.mov

Saving changes on a slow internet connection:

Screen.Recording.2023-01-03.at.21.02.24.mov

Browser is offline:

Screen.Recording.2023-01-04.at.16.31.35.mov

This bug was likely introduced when upgrading to React Router 6. The `UpdateStatus` component needed access to the router's state in order to block navigation (and show a confirmation dialog to the user) when there are unsaved changes. With the upgrade to React Router 6, this required wrapping class components in a custom `withRouter` HOC. The `UpdateStatus` component exposes the different states (`SUCCESS`, `ERROR`, `IN_PROGRESS`) as static class fields (e.g. `UpdateStatus.IN_PROGRESS`). When wrapping the component in `withRouter`, those static class fields aren't forwarded, i.e. `UpdateStatus.IN_PROGRESS` is undefined.

However, at the moment, blocking navigation isn't possible anyway (at least temporarily), as React Router v6 has removed support for relevant features required to implement this (although it will hopefully return soon [1]). So for now, I have simply removed `withRouter` altogether. Once blocking navigation is possible again, we should refactor this component to a function component and use the hooks provided by React Router.

[1] remix-run/react-router#8139
@tillprochaska tillprochaska force-pushed the till/2793-fix-entityset-status-indicator branch from 9ff583b to 1aeac51 Compare January 3, 2023 20:39
@tillprochaska tillprochaska linked an issue Jan 3, 2023 that may be closed by this pull request
Doing this as preparation to also display the browser online status.
@tillprochaska tillprochaska force-pushed the till/2793-fix-entityset-status-indicator branch from c62997d to 342c526 Compare January 4, 2023 15:47
@tillprochaska tillprochaska marked this pull request as ready for review January 4, 2023 16:02
@Rosencrantz Rosencrantz merged commit 491d412 into develop Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

BUG: Entity set status indicator always displays "Saved"
2 participants