Fix Toast Theme Support and other minor improvements #7751

MichaelBuessemeyer · 2024-04-15T11:22:40Z

This PR modifies the toast lib to render the notifications in the user-set theme. Previously, the theme was always set to light and therefore the background of the toast messages in dark mode was white. The API of the toast lib doesn't change at all in this PR.

How it works: According to the antd toast docs antd automatically creates a new rendering hierarchy for each spawned toast. For rendering the toast in the configured theme, a hook needs to be used that returns a context holder and API to open the toasts from. This is now automatically done by the Toast lib by mounting a single functional component (FC) using this hook as a direct child of the GlobalThemeProvider. Once this FC is rendered for the first time, the theme-aware notification API is available to the toast lib and can be used to render some toasts. Note, that the FC with its notification API should be treated as a singleton and only mounted once as else an old notification API might be lost with references to e.g. some older toast that are sticky and still being displayed.

URL of deployed dev instance (used for testing):

https://allowtoastoutsiderenderincontext.webknossos.xyz/

Steps to test:

Open the dev instance and switch to light theme. Then trigger some toasts via e.g. copying the position of a tracing with the pin-icon button next to the position view. The toasts should have a light background.
Switch to dark mode. And trigger some toasts again. The toasts should now be in dark mode. Switching themes while a toast is visible should also adapt the theme of the toast while it is still visible.
Open the copy position toast multiple time after another. This should always close the current toast before reopening the same toast.
While a toast like the copy position toast is visible, switch to a different browser tab. Wait 10 sec and return to the wk tab. The toast should still be visible and close after half of the timeout of the toast (likely 6 -> half is 3 secs).

Issues:

No issue exists for this.
Maybe related to Upgrade antd to v5 #6861

(Please delete unneeded items, merge only when none are left open)

~~[ ] Updated changelog~~
- I'd say that this is not necessary

…nor improvements

MichaelBuessemeyer

@philippotto Could you please review these changes as I'd say that this PR changes quite heavily how the toast lib works under the hood. The API does not change but each toast now opens a new rendering context to match the desired theme.

Sadly, these changes introduce cyclic dependencies. Please see below.

tools/check-cyclic-dependencies.js

package.json

frontend/javascripts/theme.tsx

frontend/javascripts/oxalis/model/bucket_data_handling/bounding_box.ts

frontend/javascripts/libs/toast.tsx

…ow-toast-outside-renderin-context

philippotto

thanks for fixing this 💯 I left some small comments.

frontend/javascripts/oxalis/model/bucket_data_handling/bounding_box.ts

frontend/javascripts/libs/toast.tsx

tools/check-cyclic-dependencies.js

Co-authored-by: Philipp Otto <[email protected]>

MichaelBuessemeyer

Thanks 🙏

I applied your feedback and tried to clarify why the destroy call needs to be deferred.

frontend/javascripts/libs/toast.tsx

tools/check-cyclic-dependencies.js

frontend/javascripts/libs/toast.tsx

philippotto · 2024-04-16T11:07:27Z

Then trigger some toasts via e.g. copying the position of a tracing with the pin-icon button next to the position view.

The theme looks correct now 👍 But the toast doesn't disappear on its own? The default delay is 6s. I debugged it briefly and notification.close is called, but it doesn't do anything? It works on wkorg, so it shouldn't be related to #7741 which was merged this week.

…asts - and fix manual timeout of same repeated toast

…ow-toast-outside-renderin-context

MichaelBuessemeyer

The theme looks correct now 👍 But the toast doesn't disappear on its own? The default delay is 6s. I debugged it briefly and notification.close is called, but it doesn't do anything? It works on wkorg, so it shouldn't be related to #7741 which was merged this week.

Thanks for noticing 💪. The problem was caused by the Toast.close call of the manual timeout introduced. The close call was using the global notification instance and not the one spawed by the component spawned with renderIndependently. To fix this, the toast lib would need to keep track of all rendered instances of ToastFunctionalComponentWrapper, which would make the code unnecessary complicated.

Therefore, I came up with a whole new approach by always having only one permanent mounted component wrapping the context for a single notification api.

The single mounted Component containing the Notification-/ Toast-Context is rendered as a direct child to the GlobalThemeProvider. Therefore I renamed the Wrapper Component to ToastContextMountRoot. Once the component is rendered for the first time, the Toast Object caches the notification API created by the ToastContextMountRoot and uses it for future toasts.

In case wk already triggered toasts before this component was rendered, the toast lib tracks pending toasts and shows them in bulk as soon as the notification API is available.

Moreover, I noticed two bugs with the new manual timeout approach to avoid hiding toasts during a tab not being visible (see #7741):

In case the same toast has a key and is not sticky, triggering the toast over and over 7 seconds, causes the toast to be rendered and close correctly after 6 seconds. But, the subsequent reopening toasts are closed immediately due to excessive pending close calls of the manual timeout functionality. To reproduce this behaviour just repeatedly click the "copy current position" from the position view and observe the subsequent toasts being closed immediately.
- I fixed this by caching the first async close call for the toast and ignoring future async close calls
In case a toast call doesn't a key but a manual timeout time, the toast duration would be set to 0 -> infinite / sticky and no manual timeout would be created. I fixed this. Please find my comment highlighting the fix below.

frontend/javascripts/libs/toast.tsx

MichaelBuessemeyer · 2024-04-16T13:52:03Z

frontend/javascripts/libs/toast.tsx

+    const timeOutInSeconds = timeout / 1000;
+    const useManualTimeout = !sticky && key != null;
    let toastConfig = {
      icon: undefined,
      key,
-      duration: 0,
+      duration: useManualTimeout || sticky ? 0 : timeOutInSeconds,


Here I fixed the mentioned bug that toasts with no key but a timeout would be sticky automatically, because below a manual timeout was only created if (!sticky && key != null) {....

A toast having no key might occur for a toast call with a component as a message and not a string itself where no key is set manually by the user.

MichaelBuessemeyer · 2024-04-16T13:55:01Z

frontend/javascripts/libs/toast.tsx

+    if (useManualTimeout && this.pendingTimeouts[key] == null) {
+      const timeoutToastManually = async () => {
+        const splitTimeout = timeout / 2;
+        await animationFrame(); // ensure tab is active
+        await sleep(splitTimeout);
+        await animationFrame();
+        // If the user has switched the tab, show the toast again so that the user doesn't just see the toast dissapear.
+        await sleep(splitTimeout);
+        this.close(key);
+        delete this.pendingTimeouts[key];
+      };
+      this.pendingTimeouts[key] = timeoutToastManually();


The manual timeout call is put into the pendingTimeouts object to avoid multiple timeouts leading to the buggy behaviour described above (where a repeatedly triggered to the same key closes subsequent toasts with the same key)

MichaelBuessemeyer · 2024-04-16T14:11:32Z

frontend/javascripts/theme.tsx

          }}
        >
+          {isMainProvider && <ToastContextMountRoot />}


Here the single global instance of ToastContextMountRoot is rendered

philippotto · 2024-04-16T14:47:10Z

Therefore, I came up with a whole new approach by always having only one permanent mounted component wrapping the context for a single notification api.

Great! 💯 Could you update the PR description to reflect this? A short sentence would be enough (you could link to your newer comment).

I fixed this by caching the first async close call for the toast and ignoring future async close calls

Would it simplify things when we would simply call close(key) before showing a new toast? if two toasts share the same key, it makes sense to dismiss the first one in my opinion. The current approach seems like it would ignore a second toast instead of extending its life span. Here's what I mean:

Current approach:
- open first toast (6s duration)
- after 5s, show second toast with same key (again 6s)
<--- the second toast will be ignored and after 6s in total, no toast will be shown, right?

My suggestion:
- open first toast (6s duration)
- after 5s, show second toast with same key (again 6s) <-- this closes the first toast and opens the second one
<-- after 5s, the toast will reopen. in total, the toast is shown for 11s instead of 6s

Does this make sense?

MichaelBuessemeyer · 2024-04-16T16:05:06Z

Great! 💯 Could you update the PR description to reflect this? A short sentence would be enough (you could link to your newer comment).

Sure 👍 I just edited the description mentioning that it might be important to only have one existing ToastContextMountRoot.

Would it simplify things when we would simply call close(key) before showing a new toast?

Yes, a little imo. But the previous behaviour of the notification / toast API is the one I reproduced: In case a toast with some key already existed, the call was "ignored". The toast was not closed and its timeout also wasn't refreshed.

I just investigated this further and it seems I am a little wrong: The timeout time and content of the toast is refreshed, but there is no close & open animation. See https://ant.design/components/notification#notification-demo-update for reference.

Current approach:
- open first toast (6s duration)
- after 5s, show second toast with same key (again 6s)
<--- the second toast will be ignored and after 6s in total, no toast will be shown, right?

Exactly, that how the current version operates. However, in case the content of the toast changes, it should also update in my version. But the timeout is not refreshed.

My suggestion:
- open first toast (6s duration)
- after 5s, show second toast with same key (again 6s) <-- this closes the first toast and opens the second one
<-- after 5s, the toast will reopen. in total, the toast is shown for 11s instead of 6s

Does this make sense?

Sure should I implement it exactly this way or should I leave out the closing and reopening animation?

philippotto · 2024-04-16T16:37:23Z

Sure should I implement it exactly this way or should I leave out the closing and reopening animation?

if it's easy, yes :) if not, let's keep it as is. in the end, it's not super important.

…ow-toast-outside-renderin-context

MichaelBuessemeyer · 2024-04-24T13:09:45Z

if it's easy, yes :) if not, let's keep it as is. in the end, it's not super important.

Yep, it was doable 👍 and should work now. But during testing chrome showed some weird flickering animation while opening a toast. This also appeared on older versions of the branch but not on wk.org. I couldn't track this down to a cause without a larger time investment. This flickering did not appear in Firefox as well and was also suddenly gone in the current version of the code Thus I deemed this to be a chrome hiccup or so.

Could you please look out for such an animation bug during testing? I cannot reproduce it in the current version (testing locally) but I would still like to avoid weird-looking opening animations of toasts on wk.org 😅.

Already thank you for testing in advance 🙏 🦜

frontend/javascripts/libs/toast.tsx

…ow-toast-outside-renderin-context

philippotto

Excellent! Works well for me 👍 Thanks for taking the time to fix this!

* enable proper toast support outside of component context and other minor improvements * adjust toast lib to always render in correct theme * update cyclic dependencies detection * UNDO: fix cyclic deps script output formatting * fix import order in e2e tests * Apply suggestions from code review Co-authored-by: Philipp Otto <[email protected]> * Apply pr feedback * apply pr feedback * Change to a permanent toast context root point to reenable closing toasts - and fix manual timeout of same repeated toast * update comment * reopen toast upon repetitive open call with the same key * move canceling closing of pending toast binding second wait expression --------- Co-authored-by: Philipp Otto <[email protected]>

MichaelBuessemeyer added 4 commits April 15, 2024 11:01

enable proper toast support outside of component context and other mi…

6bea52d

…nor improvements

adjust toast lib to always render in correct theme

c2d6295

update cyclic dependencies detection

d621b7c

UNDO: fix cyclic deps script output formatting

9877e47

MichaelBuessemeyer commented Apr 15, 2024

View reviewed changes

MichaelBuessemeyer added 2 commits April 15, 2024 15:51

fix import order in e2e tests

3336fc5

Merge branch 'master' of github.com:scalableminds/webknossos into all…

d8561d5

…ow-toast-outside-renderin-context

MichaelBuessemeyer self-assigned this Apr 15, 2024

MichaelBuessemeyer added enhancement frontend labels Apr 15, 2024

MichaelBuessemeyer changed the title ~~Toast support outside of component context and other minor improvements~~ Fix Toast Theme Support and other minor improvements Apr 15, 2024

philippotto requested changes Apr 15, 2024

View reviewed changes

Apply suggestions from code review

73a7d97

Co-authored-by: Philipp Otto <[email protected]>

MichaelBuessemeyer commented Apr 15, 2024

View reviewed changes

frontend/javascripts/libs/toast.tsx Outdated Show resolved Hide resolved

tools/check-cyclic-dependencies.js Outdated Show resolved Hide resolved

Merge branch 'master' into allow-toast-outside-renderin-context

38a50c9

MichaelBuessemeyer requested a review from philippotto April 16, 2024 07:59

Apply pr feedback

1d4cdd8

philippotto reviewed Apr 16, 2024

View reviewed changes

frontend/javascripts/libs/toast.tsx Outdated Show resolved Hide resolved

apply pr feedback

43a3bb0

MichaelBuessemeyer added 3 commits April 16, 2024 14:56

Change to a permanent toast context root point to reenable closing to…

0e42b2f

…asts - and fix manual timeout of same repeated toast

Merge branch 'master' of github.com:scalableminds/webknossos into all…

77823b5

…ow-toast-outside-renderin-context

update comment

4f8a2f9

MichaelBuessemeyer commented Apr 16, 2024

View reviewed changes

MichaelBuessemeyer requested a review from philippotto April 16, 2024 14:13

MichaelBuessemeyer added 2 commits April 24, 2024 14:05

Merge branch 'master' of github.com:scalableminds/webknossos into all…

0fbe1a9

…ow-toast-outside-renderin-context

reopen toast upon repetitive open call with the same key

4f28ba7

philippotto reviewed Apr 26, 2024

View reviewed changes

frontend/javascripts/libs/toast.tsx Show resolved Hide resolved

MichaelBuessemeyer added 2 commits April 30, 2024 11:53

move canceling closing of pending toast binding second wait expression

e03c13e

Merge branch 'master' of github.com:scalableminds/webknossos into all…

f2f86f5

…ow-toast-outside-renderin-context

MichaelBuessemeyer requested a review from philippotto April 30, 2024 09:57

philippotto approved these changes Apr 30, 2024

View reviewed changes

MichaelBuessemeyer merged commit 098413d into master Apr 30, 2024
2 checks passed

MichaelBuessemeyer deleted the allow-toast-outside-renderin-context branch April 30, 2024 11:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Toast Theme Support and other minor improvements #7751

Fix Toast Theme Support and other minor improvements #7751

MichaelBuessemeyer commented Apr 15, 2024 •

edited

Loading

MichaelBuessemeyer left a comment

philippotto left a comment

MichaelBuessemeyer left a comment

philippotto commented Apr 16, 2024

MichaelBuessemeyer left a comment

MichaelBuessemeyer Apr 16, 2024

MichaelBuessemeyer Apr 16, 2024

MichaelBuessemeyer Apr 16, 2024

philippotto commented Apr 16, 2024

MichaelBuessemeyer commented Apr 16, 2024

philippotto commented Apr 16, 2024

MichaelBuessemeyer commented Apr 24, 2024

philippotto left a comment

Fix Toast Theme Support and other minor improvements #7751

Fix Toast Theme Support and other minor improvements #7751

Conversation

MichaelBuessemeyer commented Apr 15, 2024 • edited Loading

URL of deployed dev instance (used for testing):

Steps to test:

Issues:

MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

philippotto left a comment

Choose a reason for hiding this comment

MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

philippotto commented Apr 16, 2024

MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

MichaelBuessemeyer Apr 16, 2024

Choose a reason for hiding this comment

MichaelBuessemeyer Apr 16, 2024

Choose a reason for hiding this comment

MichaelBuessemeyer Apr 16, 2024

Choose a reason for hiding this comment

philippotto commented Apr 16, 2024

MichaelBuessemeyer commented Apr 16, 2024

philippotto commented Apr 16, 2024

MichaelBuessemeyer commented Apr 24, 2024

philippotto left a comment

Choose a reason for hiding this comment

MichaelBuessemeyer commented Apr 15, 2024 •

edited

Loading