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
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
native/chat/chat-input-bar.react.js | ||
---|---|---|
469–470 ↗ | (On Diff #45949) | I spent some time investigating why this was removed. (In the future, it would be helpful if you explain your reasoning around changes like this.) From reading the code, it looks like the purpose was to handle a scenario where the user goes from not being voiced to being voiced. It looks like the send button is only rendered when the user is voiced, so I guess the idea is that we want to "reset" the send button animation state before we start rendering it. I analyzed whether this reset is really necessary:
With all that said, I think the most "correct" thing to do here if we wanted to be safe would be: updateSendButton(currentText: string) { const targetValue = currentText === '' ? 0 : 1; this.props.targetSendButtonContainerOpen.setValue(targetValue); if (!this.shouldShowTextInput) { this.props.sendButtonContainerOpen.setValue(targetValue); } } This feels more "correct" to me... basically what we're saying is that we don't want an animation to run when the send button is not being shown, we just want to immediately update its position to where it should be, so it's in the right place if it starts being rendered. |
1286–1295 ↗ | (On Diff #45949) | What do you think of this shorthand? |
1331 ↗ | (On Diff #45949) | I don't think this should be in the dep list. Doesn't it result in everything being reset whenever initialSendButtonContainerOpen changes? You can use eslint-disable like so to disable the ESLint rule: // eslint-disable-next-line react-hooks/exhaustive-deps |