New updates toast message only when button is displayed #5306

acelaya · 2023-03-09T09:02:23Z

This PR changes the logic introduced in #5305 to make sure the message is toasted only when pending updates goes from 0 to a value greater than zero, which effectively means it is toasted when the button to refresh notifications is displayed.

If for any reason the pending notifications are increased before the notifications are refreshed, this prevents the message to be toasted again, which might be overwhelming on documents with a lot of activity.

Testing steps:

Change to this branch on your local client instance.
Open it using your local network address (192.168.x.x:3000), not localhost:3000.
Make sure you are logged in and the sidebar is "focused".
Run a screen reader service.
Open the same network address on a different device which is on the same network (I have been testing it with my mobile phone).
Login and add an annotation on the second device: The screen reader on the first device should announce that there's a new annotation available.
Before clicking on the refresh button, add another annotation from the second device: The screen reader should not announce anything.
Click on refresh annotations on the first device.
Add another annotation from the second device: The screen reader should announce that there are new annotations available.

codecov · 2023-03-09T09:06:07Z

Codecov Report

Merging #5306 (5e64dc2) into main (ffdc0b3) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #5306   +/-   ##
=======================================
  Coverage   99.17%   99.17%           
=======================================
  Files         235      236    +1     
  Lines        9345     9354    +9     
  Branches     2241     2241           
=======================================
+ Hits         9268     9277    +9     
  Misses         77       77

Impacted Files	Coverage Δ
src/sidebar/components/TopBar.tsx	`100.00% <ø> (ø)`
src/sidebar/components/PendingUpdatesButton.tsx	`100.00% <100.00%> (ø)`
src/sidebar/store/modules/real-time-updates.js	`100.00% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

src/sidebar/components/TopBar.tsx

lyzadanger

Getting real close here, I think!

I had a couple of suggestions...

lyzadanger · 2023-03-09T21:04:54Z

src/sidebar/components/PendingUpdatesButton.tsx

+  useEffect(() => {
+    if (hasPendingUpdates) {
+      toastMessenger.success(
+        `There are ${pendingUpdateCount} new annotations.`,


Because we will only announce once and not again until any additional annotations are loaded in, I think we might consider not announcing the specific count, as it may not be accurate by the time the user loads in the annotations.

Suggested change

`There are ${pendingUpdateCount} new annotations.`,

`New annotations are available`,

This will read better when it can be followed with "Press (whatever) to load these annotations". And even better down the line when we can confirm with "X new annotations loaded".

BTW, I see now why you were a little ambivalent about adding a hasPendingUpdates selector, since this component also needs pendingUpdateCount. But I still think it's fine...

BTW, I see now why you were a little ambivalent about adding a hasPendingUpdates selector, since this component also needs pendingUpdateCount. But I still think it's fine...

Yes. I mean, we could have a locally memoized flag which depends on the amount. Something in these lines:

const hasPendingUpdates = useMemo(() => pendingUpdateCount > 0, [pendingUpdateCount]);

But if we are going to do that we may as well have the selector, which is easier to reason about and can be used somewhere else if needed.

lyzadanger · 2023-03-09T21:06:26Z

src/sidebar/components/TopBar.tsx

-                  }`}
-                />
-              )}
+              <PendingUpdatesButton onClick={applyPendingUpdates} />


Given that PendingUpdatesButton is already pretty self sufficient and makes use of the toastMessengerService, I wonder if you might go ahead and inject the streamer service into it, and free TopBar from having to know about streamer. Thoughts?

Good point!

lyzadanger · 2023-03-09T21:09:11Z

src/sidebar/components/PendingUpdatesButton.tsx

+    // We only want this effect to trigger when changing from no-updates to at
+    // least one update, not every time the amount changes
+    // eslint-disable-next-line react-hooks/exhaustive-deps


I think this may be out of date. Can you check the dependencies here?

acelaya · 2023-03-10T08:26:37Z

Thanks @lyzadanger! All suggestions pushed.

lyzadanger · 2023-03-10T13:19:26Z

@acelaya In the hopes that this will make your testing strategy easier: you don't have to use different devices to test this feature. All you need is a separate session (browser profile, different browser, e.g.). For example, here is a screen shot of me using:

localhost in both cases
Chrome, logged in as devdata_admin (this is a user you get for free with devdata)
Safari, logged in as another user (I created this one at some point). With this user, I create a new annotation. My Chrome session (open on the same document) then sees the "pending-annotations" button

I hope this makes your life a little easier?

(Safari here is on top of the Chrome session)

Edited: Oh wait, I think I get it. It's not possible to retain focus on the iframe that would announce the pending update in this setup. Disregard!

acelaya · 2023-03-10T13:28:12Z

Oh wait, I think I get it. It's not possible to retain focus on the iframe that would announce the pending update in this setup. Disregard!

Exactly 😅.

I used this approach at first, but I had to end up adding a small timeout to dispatching the reduce action so that I had time to go back to the main window before it got actually dispatched, otherwise the screen reader would not announce anything.

At some point it became cumbersome to have to remember which file needed to be excluded from commits, plus I realized I was not testing the real behavior and thought I might miss something. That's when I moved to use two devices.

lyzadanger

SO so so so so so close! Looking good! There is one thing I'd like to address: ToastMessageItem will prefix success or error toast messages with the word Success: or Error: respectively. If you set the message as a notice type, it shouldn't do that prefixing (right now, the word Success: is announced as part of the message: let's get rid of that). I think these are "notice" type messages, anyway.

Everything else is working great...let's get this done!

lyzadanger · 2023-03-10T12:18:51Z

src/sidebar/components/PendingUpdatesButton.tsx

+  toastMessenger: ToastMessengerService;
+  streamer: StreamerService;


World's teensiest nit: I tend to alphabetize injected service props.

lyzadanger · 2023-03-10T12:22:45Z

src/sidebar/store/modules/real-time-updates.js

@@ -217,6 +217,17 @@ function hasPendingDeletion(state, id) {
  return hasOwn(state.pendingDeletions, id);
 }

+/**
+ * Return true if an annotation has been created on the server, but it has not
+ * yet been applied.


The wording "applied" is somewhat vague. I imagine you derived this comment from elsewhere: don't worry about it for now, and I can see why applied could make sense (I mean, the annotations are loaded already in a sense...). This makes me realize that I should familiarize myself with the flow of "pending" annotations a little better: then I might be able to provide a suggestion...

Yes, I kept "applied" for consistency with other comments, but it was not super obvious to me neither, to be honest. I wouldn't mind finding a better wording.

acelaya · 2023-03-10T13:49:50Z

@lyzadanger changed to use notice toast messages instead of success ones, and ordered the injected services.

…ressed

…onClick prop

lyzadanger

All righty, let's get this landed!

acelaya requested a review from lyzadanger March 9, 2023 09:02

acelaya mentioned this pull request Mar 9, 2023

Publish hidden toast message when there are new pending updates #5305

Closed

5 tasks

acelaya commented Mar 9, 2023

View reviewed changes

src/sidebar/components/TopBar.tsx Outdated Show resolved Hide resolved

acelaya linked an issue Mar 9, 2023 that may be closed by this pull request

Screen reader does not announce when new/updated annotations are available hypothesis/product-backlog#1422

Closed

acelaya mentioned this pull request Mar 9, 2023

Add keyboard shortcut to load pending updates #5307

Closed

acelaya force-pushed the new-annotations-toast-throttle branch from bafdba2 to cce0dd7 Compare March 9, 2023 13:29

lyzadanger reviewed Mar 9, 2023

View reviewed changes

acelaya requested a review from lyzadanger March 10, 2023 08:35

lyzadanger suggested changes Mar 10, 2023

View reviewed changes

acelaya changed the base branch from new-annotations-toast to main March 10, 2023 13:40

acelaya force-pushed the new-annotations-toast-throttle branch from c74451f to ac096f8 Compare March 10, 2023 13:49

acelaya requested a review from lyzadanger March 10, 2023 13:49

acelaya mentioned this pull request Mar 14, 2023

Use PendingUpdatesButton in notebook #5322

Merged

acelaya force-pushed the new-annotations-toast-throttle branch from ac096f8 to 61b34e7 Compare March 14, 2023 13:43

acelaya added 6 commits March 15, 2023 12:41

Publish hidden toast message when there are new pending updates

53bafa9

Test accessible message toasted on new annotations in TopBar

18e87b4

Extract PendingUpdatesButton to its own component

019980a

Make sure pending updates are toasted only once until the button is p…

144eaba

…ressed

Test hasPendingUpdates selector

4a472c6

Make PendingUpdatesButton wrap streamer service instead of expect an …

5e64dc2

…onClick prop

acelaya force-pushed the new-annotations-toast-throttle branch from 61b34e7 to 5e64dc2 Compare March 15, 2023 11:41

lyzadanger approved these changes Mar 15, 2023

View reviewed changes

acelaya merged commit 63897c3 into main Mar 15, 2023

acelaya deleted the new-annotations-toast-throttle branch March 15, 2023 13:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New updates toast message only when button is displayed #5306

New updates toast message only when button is displayed #5306

acelaya commented Mar 9, 2023 •

edited

Loading

codecov bot commented Mar 9, 2023 •

edited

Loading

lyzadanger left a comment

lyzadanger Mar 9, 2023

lyzadanger Mar 9, 2023

acelaya Mar 10, 2023

lyzadanger Mar 9, 2023

acelaya Mar 10, 2023

lyzadanger Mar 9, 2023

acelaya commented Mar 10, 2023

lyzadanger commented Mar 10, 2023 •

edited

Loading

acelaya commented Mar 10, 2023

lyzadanger left a comment

lyzadanger Mar 10, 2023

lyzadanger Mar 10, 2023

acelaya Mar 10, 2023

acelaya commented Mar 10, 2023

lyzadanger left a comment

	`There are ${pendingUpdateCount} new annotations.`,
	`New annotations are available`,

		toastMessenger: ToastMessengerService;
		streamer: StreamerService;

New updates toast message only when button is displayed #5306

New updates toast message only when button is displayed #5306

Conversation

acelaya commented Mar 9, 2023 • edited Loading

Testing steps:

codecov bot commented Mar 9, 2023 • edited Loading

Codecov Report

lyzadanger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acelaya commented Mar 10, 2023

lyzadanger commented Mar 10, 2023 • edited Loading

acelaya commented Mar 10, 2023

lyzadanger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acelaya commented Mar 10, 2023

lyzadanger left a comment

Choose a reason for hiding this comment

acelaya commented Mar 9, 2023 •

edited

Loading

codecov bot commented Mar 9, 2023 •

edited

Loading

lyzadanger commented Mar 10, 2023 •

edited

Loading