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

Add new notification displayed when there are pending updates #6256

Merged
merged 1 commit into from
Mar 18, 2024

Conversation

acelaya
Copy link
Contributor

@acelaya acelaya commented Mar 14, 2024

Part of #6255

This PR adds the initial logic to display a toast-like notification when new updates are available (new annotations, updated annotations, etc).

The notification is fully displayed for 5 seconds, and then collapsed to be less intrusive. When hovered it gets uncollapsed. When clicked, it loads all pending updates.

The notification replaces the existing PendingUpdatesButton, and is currently handled by a feature flag.

notification-2024-03-15_15.32.30.mp4

Testing steps

  1. Check out this branch
  2. Open http://localhost:3000 in two different browsers
  3. Create an annotation in one of them. The other one should show the existing PendingUpdatesButton.
  4. Go to http://localhost:5000/admin/features and enable the pending_updates_notification feature.
  5. Repeat step 3 and check that the new notification is displayed this time, instead of the PendingUpdatesButton
  6. If you wait for ~5 seconds, the notification should collapse to show only the icon and number of updates.
  7. If you click the notification, it should load the new updates.

Out of scope

Some things will be eventually implemented but are out of the scope of this specific PR:

  • The example in https://codepen.io/jaredpdesigns/full/bGQRKwO uses a slide-from-right animation when the notification is presented. I just added a fade-in because the existing slide-in-from-right one expects the element to be positioned based on the left side, so we would need a variation of it.
  • The example also shows a behavior in which new updates are not "appended" right away, but debounced. I left that out of the scope of this PR for now, so it updates based on how new stuff is streamed.
  • The notifications count is incremented with a slide-up animation. I did not implement that here.
  • When the notification is clicked, the design shows how the user is scrolled to the new annotations. That is not implemented here, as it has several considerations:
    • We need to decide to which annotation we scroll, as they could be in a few places.
    • We need to decide if there's some case in which the user should not be scrolled to the new annotations, for example if an annotation creation/edition is in progress.

Considerations

  • The icon in the notification is the same we have in the existing button, as we don't have one like in the designs.

TODO

  • Evaluate final color for notification
  • Make notification "collapse" after a couple of seconds, and fully show on hover
  • Implement notification in notebook
  • Add tests

@acelaya acelaya force-pushed the new-annos-notification branch 9 times, most recently from 32751cb to d41f6b8 Compare March 15, 2024 14:53
Copy link

codecov bot commented Mar 15, 2024

Codecov Report

Attention: Patch coverage is 95.23810% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 99.43%. Comparing base (3ad17ab) to head (5b6d1b3).

❗ Current head 5b6d1b3 differs from pull request most recent head 7a76ae3. Consider uploading reports for the commit 7a76ae3 to get more accurate results

Files Patch % Lines
.../sidebar/components/PendingUpdatesNotification.tsx 94.11% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6256      +/-   ##
==========================================
- Coverage   99.44%   99.43%   -0.02%     
==========================================
  Files         266      267       +1     
  Lines       10135    10177      +42     
  Branches     2400     2418      +18     
==========================================
+ Hits        10079    10119      +40     
- Misses         56       58       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@acelaya acelaya force-pushed the new-annos-notification branch 6 times, most recently from f4ff71d to 17b1b77 Compare March 18, 2024 14:03
@acelaya acelaya marked this pull request as ready for review March 18, 2024 14:05
@acelaya
Copy link
Contributor Author

acelaya commented Mar 18, 2024

Codecov is marking as not covered two lines annotated with istanbul ignore next 🤔

@acelaya acelaya requested a review from robertknight March 18, 2024 14:14
@acelaya acelaya force-pushed the new-annos-notification branch from 17b1b77 to b00e30e Compare March 18, 2024 14:28
@robertknight
Copy link
Member

I found an issue where the notice does not appear when there are pending deletions but no pending updates.

  1. Create two annotations
  2. Open two tabs
  3. Delete one annotation in tab A. No notification appears in the other tab.
  4. Update the other annotation in tab A. A notification appears in the other tab with the text "Load 2 updates"

@acelaya
Copy link
Contributor Author

acelaya commented Mar 18, 2024

I found an issue where the notice does not appear when there are pending deletions but no pending updates.

  1. Create two annotations
  2. Open two tabs
  3. Delete one annotation in tab A. No notification appears in the other tab.
  4. Update the other annotation in tab A. A notification appears in the other tab with the text "Load 2 updates"

I experienced the same during testing, but I thought this was by design, because the existing PendingUpdatesButton behaves the same.

@acelaya acelaya force-pushed the new-annos-notification branch from b00e30e to 5b6d1b3 Compare March 18, 2024 15:23
@robertknight
Copy link
Member

I experienced the same during testing, but I thought this was by design, because the existing PendingUpdatesButton behaves the same.

I'm can't remember if that was intentional or not. Looking at the behavior with fresh eyes, I think it would be better to change this so that deletions, additions and updates are all handled the same way.

Copy link
Member

@robertknight robertknight left a comment

Choose a reason for hiding this comment

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

This looks good. I provided some feedback on handling of deletions but I think that can be addressed in a subsequent PR.

src/sidebar/components/PendingUpdatesNotification.tsx Outdated Show resolved Hide resolved
src/sidebar/components/PendingUpdatesNotification.tsx Outdated Show resolved Hide resolved
src/sidebar/components/PendingUpdatesNotification.tsx Outdated Show resolved Hide resolved
assert.calledOnce(fakeSetTimeout);

await promise; // Wait for timeout callback to be invoked
wrapper.update();
Copy link
Member

Choose a reason for hiding this comment

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

wrapper.update's main function is to sync the wrapper with the rendered DOM, which is not immediately updated after the timeout fires, however it does also flush any pending state updates first. Since the state update is queued inside the timeout callback, this is OK.

@acelaya
Copy link
Contributor Author

acelaya commented Mar 18, 2024

I'm can't remember if that was intentional or not. Looking at the behavior with fresh eyes, I think it would be better to change this so that deletions, additions and updates are all handled the same way.

Agreed. I found it a bit counter-intuitive when I saw it.

@acelaya acelaya force-pushed the new-annos-notification branch from 5b6d1b3 to 7a76ae3 Compare March 18, 2024 15:58
@acelaya acelaya merged commit f72292d into main Mar 18, 2024
2 checks passed
@acelaya acelaya deleted the new-annos-notification branch March 18, 2024 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants