Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unify the two platform's ComposeBox code #3095

Merged
merged 3 commits into from
Nov 1, 2018

Conversation

borisyankov
Copy link
Contributor

@borisyankov borisyankov commented Nov 1, 2018

Closes #3017. Fixes #2434. Fixes #3053.

Remove the iOS code which was our old 'controlled' version and
keep the new Android 'uncontrolled' version.

While iOS does not have the extreme performance issues the Android
app had because of bugs in React Native, it is still faster to not
have the component controlled - a controlled component would block
any time the UI thread blocks.

Another big benefit is removing the need to support two versions.

@gnprice gnprice added the review label Nov 1, 2018
@borisyankov borisyankov changed the title Unify two platform's ComposeBox code. Unify the two platform's ComposeBox code Nov 1, 2018
The logic here is already correct, with a condition just under this;
just need to make the signature agree.

This issue is found by Flow when we unify the ComposeBox files,
coming soon -- Flow unfortunately doesn't check *.android.js.
borisyankov and others added 2 commits October 31, 2018 18:08
Closes zulip#3017.  Fixes zulip#3053.

Remove the iOS code which was our old 'controlled' version and
keep the new Android 'uncontrolled' version.

While iOS does not have the extreme performance issues the Android
app had because of bugs in React Native, it is still faster to not
have the component controlled - a controlled component would block
any time the UI thread blocks.

Another big benefit is removing the need to support two versions.
Resolving these `any`s into the proper type shows a couple of spots
where we should be checking for null.  Not sure if these cases can
actually be exercised to cause a live bug -- but this is the easiest
way to be sure.

Also remove the `MultilineInput#textInput` property, which doesn't
seem to be ever set; presumably left behind by a past refactor.
@gnprice gnprice merged commit afdb464 into zulip:master Nov 1, 2018
@gnprice gnprice removed the review label Nov 1, 2018
@gnprice
Copy link
Member

gnprice commented Nov 1, 2018

Merged -- thanks @borisyankov !

I believe #2434 was actually fixed by the RN v0.57 upgrade earlier today -- but the difference doesn't matter, of course. Glad they're all fixed. 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants