Details
Tested this diff stack by playing around with ChatInputBar on both iOS simulator and Android device:
- focus the text input and verify the keyboard is up and the camera buttons are animated correctly
- try to send the message, verify the send button is animated correctly
- verify the text input can be unfocused and keyboard is hidden
- verify the draft works: write text, navigate from the chat, navigate into the chat again, verify the text is kept and the $
- verify the edit mode works by editing a message
- try to close the chat while editing a message - the alert should be shown
- try to join a thread
- try to select typed text
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
I don't think the way you're using the dep lists is safe
| native/chat/chat-input-bar.react.js | ||
|---|---|---|
| 1347 ↗ | (On Diff #45957) | Nit: we always use ID instead of Id |
| 1377 ↗ | (On Diff #45957) | I don't think this is safe... the effect won't re-run when addEditInputMessageListener changes, which means addEditInputMessageListener can be bound with an out-of-date version of focusAndUpdateText, which might eg. have an out-of-date version of text. It's probably best to avoid eslint-disable-next-line and use a prevIsActiveRef ref variable to make sure that the callbacks are only called when isActive changes. |
| 1392 ↗ | (On Diff #45957) | This seems similarly sketchy, and because you have prevTextRef, it's not clear to my why it's necessary at all. Why not use the proper dep list? |
| 1395–1419 ↗ | (On Diff #45957) | Same concerns |
Please do not land this without addressing the typo
| native/chat/chat-input-bar.react.js | ||
|---|---|---|
| 1379 ↗ | (On Diff #46033) | There's a typo here... you missed .current |
| native/chat/chat-input-bar.react.js | ||
|---|---|---|
| 1379 ↗ | (On Diff #46033) | This feedback was not responded to. It continues a pattern of landing diffs without responding to feedback. I was worried about this happening again, which is why I said "Please do not land this without addressing the typo". I suppose that too was missed... Let's discuss this ASAP... given we've already discussed twice, at this point I think it might be best to introduce a rule on the team where reviewers of your diffs don't accept until all questions and feedback are resolved. |