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 |
turt2live
added
the
X-Release-Blocker
jryans
left a comment
jryans
left a comment
There was a problem hiding this comment.
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.
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.
(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.
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 |
3 participants
Design review portion
Is this copy good enough?
... then when the user manages to
resendeverything ...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.mddocs and then reviewing the code.Live demo:
TODO: