-
-
Notifications
You must be signed in to change notification settings - Fork 829
Add local echo for notifications in the new room list #5065
Conversation
This is somewhat expected to be temporary.
In some cases we're likely to miss the PREPARED sync, so just handle ourselves as ready if we have a client set.
The structure here might need some documentation and work, but overall the idea is that all calls pass through a CachedEcho instance, which are self-updating.
This presumes success as we don't yet have a UI for failures.
The EchoTransaction was wrongly assuming that it knew better than the caller for when the success condition was met, so the echo marking has been left an exercise for the caller. In this case, we mark when we finally receive the sync with the updated rules. We also have to cancel previous transactions otherwise if the user mashes buttons we could forever show the toast, and that would be bad.
Copy reviewed oob |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like an interesting approach, let's give it a try!
I'm not quite sure why you've marked this as needing to be in the RC though...? It seems like a new feature that can merge at any time.
return Array.from(this.toasts.values()); | ||
} | ||
|
||
public addToast(c: ComponentClass): ToastReference { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than storing a class to eventually create, it might be more natural to let the caller pass a normal fully created (but unmounted) component (as a block of JSX most likely), which is then not mounted until needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I'm not trying to say anything needs to be changed here, just reflecting on what might be more natural to use.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea, it's a bit awkward how we create all this stuff. Something to review for toasts in general.
Since this is relatively isolated to notification state for the moment, it seems safe enough to land and try for the upcoming RC, so I'll merge this now. Please do look over the comments above and consider fixing after the fact where needed. |
It fixes a regression where the notifications menu appears to do nothing. We could in theory try and fix it, but notifications are so badly scoped that either the work would be undone by ftue notifications or we'd regress performance. Local echo is just one of the ways out of both problems. |
lgtm for first iteration |
Design review portion
Is this copy good enough?
... then when the user manages to
resend
everything ...Code review portion
For element-hq/element-web#14302
Fixes element-hq/element-web#14280
This is somewhat reviewable commit-by-commit, though I'd instead recommend reading the included
local-echo-dev.md
docs and then reviewing the code.Live demo:
TODO: