webknossos icon indicating copy to clipboard operation
webknossos copied to clipboard

Fix Toast Theme Support and other minor improvements

Open MichaelBuessemeyer opened this issue 10 months ago • 5 comments

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 #6861

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

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

MichaelBuessemeyer avatar Apr 15 '24 11:04 MichaelBuessemeyer

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 :+1: 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 https://github.com/scalableminds/webknossos/pull/7741 which was merged this week.

philippotto avatar Apr 16 '24 11:04 philippotto

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! :100: 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?

philippotto avatar Apr 16 '24 14:04 philippotto

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

Sure :+1: 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?

MichaelBuessemeyer avatar Apr 16 '24 16:04 MichaelBuessemeyer

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.

philippotto avatar Apr 16 '24 16:04 philippotto

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 :+1: 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 :sweat_smile:.

Already thank you for testing in advance :pray: :parrot:

MichaelBuessemeyer avatar Apr 24 '24 13:04 MichaelBuessemeyer