Make ComposeBox message and topic inputs uncontrolled #2738
Conversation
borisyankov
force-pushed
the
better-composebox
branch
from
2cc51e7
to
79007d6
Compare
borisyankov
changed the title
borisyankov
changed the title
borisyankov
force-pushed
the
better-composebox
branch
from
79007d6
to
11532f7
Compare
|
Hello @borisyankov, it seems like you have referenced an issue in your pull request, but you have not referenced that issue in your commit message(s). When you reference an issue in a commit message, it automatically closes the corresponding issue when the commit is merged. Please run An example of a correctly-formatted commit: Thank you for your contributions to Zulip! |
borisyankov
force-pushed
the
better-composebox
branch
2 times, most recently
from
79007d6
to
62c291d
Compare
There was a problem hiding this comment.
Thanks @borisyankov for all these changes! This is very close. I'm now much happier with how easily I (and future readers) can understand what's going on in these changes.
One comment below, where I'm still not quite sure what the story is intended to be. I also made one comment on #2739, which you sent with the first two commits of this branch.
One other thing, which should be straightforward to fix: the commit that splits into two files makes the iOS file equal the old version, instead of the new version after the first couple of commits that were intended to apply to both.
src/compose/ComposeBox.android.js
Outdated
| return; | ||
| } | ||
|
|
||
| textInput.setNativeProps({ text }); |
There was a problem hiding this comment.
This line appears in the commit that first introduces updateTextInput, while the inputs are still controlled. I'm still a bit confused about exactly what it does.
In the case where the input is controlled, is it your understanding that this line has an effect on behavior? Or does it not have an effect for some reason?
If it doesn't have an effect, I'd appreciate an explanation of that in the commit message -- or even just a mention of it. Even just knowing that that was your intention when writing the commit is very helpful for trying to understand it.
If it does have an effect on behavior, what is that effect?
borisyankov
force-pushed
the
better-composebox
branch
from
62c291d
to
806c554
Compare
|
Heads up @borisyankov, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the |
We want to make the compose box uncontrolled. Instead of using the `value` property to change the message input's and topic input's text, we will call `setNativeProps` on a reference to the underlying control. Currently on iOS there is a React Native bug that affects uncontrolled inputs: facebook/react-native#18272 On the other hand, in order to fix zulip#2589 we're eager to switch to an uncontrolled input at least on Android, in order to avoid the (different) underlying React Native bug that causes that. So for now, as a suboptimal solution, we will support two versions of the component. For iOS it will remain unchanged, and we'll modify the Android version of the file in subsequent commits.
This will let us re-use the same logic for the topic input, and for
initializing the inputs on mount when we make them uncontrolled.
While we're here, add some comments explaining the trickier bits
of this logic.
We also add a `setMessageInputValue` wrapper that also makes sure to
call the event handler `handleMessageChange`, for consistency in
behavior with what gets called when a user types text themselves.
The new function adds a call to `setNativeProps`. This doesn't have
any effect as long as the input is controlled, and it makes a simpler
transition to the uncontrolled version coming next.
The way controlled/uncontrolled state works in TextInput is:
When controlled (and without this `setNativeProps`):
* we set message in state
* invisibly to us, RN calls setNativeProps to reconcile the
difference between what value we want and what is the current
value of the native component
When uncontrolled:
* we manually call setNativeProps
* we also set state for `message`, but that is only so we track
the current value of the input in JS-land
In this commit:
* we call setNativeProps with value X
* we also set state to X
* React Native invisibly to us checks if desired value differs
from what the value of the native component is. X === X so
RN does not do anything.
Remove the `value={message}` from message text input, making it
an uncontrolled component.
Then do the following changes to reimplement previous behavior:
* at the very start in `componentDidMount`, make sure we update
the component with the initial values (coming from state)
* ensure that autocomplete changes the input values
* on editing a message, set the input text to the previous
message contents
Remove the `value={topic}` from topic text input, making it
an uncontrolled component.
To replicate the previous behavior:
* initialize at the start in `componentDidMount`
* make sure the topic input text is updated when we focus on
the message input (it is invisible until then)
* change the text on picking a topic autocomplete suggestion
Fixes zulip#2589.
gnprice
force-pushed
the
better-composebox
branch
from
806c554
to
1f9ad56
Compare
|
Merged. Thanks for all the details added to the I'll go build a 14.x release with this. |
Fixes #2589
When having 'controlled' TextInput components we run the risk of
JS blocking the updates to these components while you type.
This is generally not an issue in web React and rarely in React Native
(communication through the bridge is fast but slower than not having it at all)
Some users have experienced unusually slow input in the compose box.
This is likely related to facebook/react-native#19126
Instead of using the topic and message inputs' property value
we can short-circuit this and update native properties ourselves.
This was an improvement I wanted to implement previously even before the
issues were reported, but back then it wasn't worth the effort.