quote-and-reply: Set topic input when appropriate; focus message input #5723
Conversation
chrisbobbe
added
a-compose/send
chrisbobbe
force-pushed
the
pr-quote-and-reply-topic
branch
from
a0c19b0 to
eebcb06
Compare
src/compose/ComposeBox.js
Outdated
| } finally { | ||
| setActiveQuoteAndRepliesCount(v => v - 1); | ||
| activeInvocations.current = activeInvocations.current.filter(x => x !== serialNumber); | ||
| messageInputRef.current?.focus(); |
There was a problem hiding this comment.
What's the reasoning for putting this step in the finally block?
I think it should instead go at the end of the try block. The existing job of this finally block is purely about making sure that the setActiveQuoteAndRepliesCount(v => v + 1) and activeInvocations.current.push at the top of the function always get closed eventually, regardless of whether the invocation succeeds or fails. If it does fail, I think it's probably not helpful to go and focus the input — we haven't successfully set up something for the user to type their reply after.
There was a problem hiding this comment.
Oh interesting, yeah, I agree it seems better to put it at the end of the try. Will test to make sure that doesn't break anything.
There was a problem hiding this comment.
Yep, that seems to work fine. Thanks for the catch!
We'll use topicSelectionAllowed soon, for zulip#5718.
Note that we have logic in `handleMessageFocus` to respond to focus on the message input by auto-focusing the *topic* input in some cases. That would be annoying if it happened here: you do a quote-and-reply, and suddenly the topic input is focused?(!) But that topic auto-focusing should be defeated in all cases where it was active. In particular, since the previous commit, the topic input won't be empty; the logic in `handleMessageFocus` will see that and skip the auto-focus. Fixes: zulip#5718
chrisbobbe
force-pushed
the
pr-quote-and-reply-topic
branch
from
eebcb06 to
e7d3d81
Compare
|
Thanks for the review! Revision pushed. |
|
Thanks! Looks good; merging. |
Fixes: #5718