-
-
Notifications
You must be signed in to change notification settings - Fork 833
Improve message sending states to match new designs #5699
Conversation
f43b301
to
db89785
Compare
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.
Overall, this side seems reasonable enough, and is trying hard to avoid extra work where possible. However, I am curious if the JS side can be more efficient, and that may change this side, so only a general comment for now.
Making this more efficient does indeed become difficult, and I'm not sure it would be a maintainable solution (would feel too much like magic/react abuse). The extra loops here should be hitting highly cached data and be extremely quick, and aren't even triggered in most cases. In testing with a medium-sized account, it seemed to perform reasonably well. |
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.
Okay, let's proceed with approach and watch out for perf impact.
Since this would go live immediately, it also needs Design review. (Perhaps faster to nudge the designer you are working with directly.)
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.
There might have been some miscommunication with the behaviour of the sent tick. Basically, in your implementation, you see a tick when your message is sent. if you write a new message you see another tick. For each message you sent there's a tick next to it. What we actually want is: Display a tick once a message is sent. Once a new message is sent, the tick from the message above should disappear. So essentially you only really see one tick at a time.
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.
Looks good overall, thanks! 😄
For element-hq/element-web#16424
Requires matrix-org/matrix-js-sdk#1625Built on top of #5697 - see pull/5697/head...pull/5699/head for the real diffTODO: