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

Remove touch events and external keyboard dependency #327

Closed
wants to merge 5 commits into from

Conversation

tafelito
Copy link
Contributor

@tafelito tafelito commented Jan 4, 2017

I removed the dependency for react-native-dismiss-keyboard and removed the touch events. Instead of using touch events and dismiss manually the keyboard, we can remove keyboardShouldPersistTaps and it has the same effect.

@tafelito
Copy link
Contributor Author

tafelito commented Jan 4, 2017

I also changed RN version, but there was an error 0.39 in Android so I downgraded to 0.37 again. I already tested in 0.39.2 and it's working for both iOS and Android.

Copy link
Contributor

@dgdavid dgdavid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that is better to do the react-native upgrade in an isolated pull request because we should review the breaking changes.

@dgdavid
Copy link
Contributor

dgdavid commented Jan 5, 2017

Sorry @tafelito, I'm sleeping yet. I didn't notice that you were upgrading the example app. Anyway, the breaking changes need to be reviewed. Why not split the PR and update first the example app?

@kfiroo
Copy link
Collaborator

kfiroo commented Jan 5, 2017

@dgdavid You are right, however it's only the example deps.
@tafelito If you are down to split the PRs that would be awesome, otherwise I think it's fine

@tafelito
Copy link
Contributor Author

tafelito commented Jan 5, 2017

@dgdavid @kfiroo The reason why I upgraded the example is because it was not working for me on the previuos version. I read of the issues posted here that upgrading to 0.37 and 15.3.2 solved the issue so I did that.

With 0.30 there is this error

...react-native-gifted-chat/example/node_modules/react-native/Libraries/WebSocket/RCTSRWebSocket.m:1334:5: error: ignoring return value of function declared with warn_unused_result attribute [-Werror,-Wunused-result]
    SecRandomCopyBytes(kSecRandomDefault, sizeof(uint32_t), (uint8_t *)mask_key);
    ^~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 errors generated.

Any ideas how to solve that using 0.30?

@tafelito
Copy link
Contributor Author

tafelito commented Jan 11, 2017

@kfiroo I saw you merged a PR where you use the keyboardShouldPersistTaps={always} and I wanted to ask you the reason behind this. It's still using a dependency on a third party plugin to dismiss the Keyboard while there is no need if you don't set that property at all. Maybe I'm missing something but I cannot see what.

Thanks

@kfiroo
Copy link
Collaborator

kfiroo commented Jan 12, 2017

@tafelito Hey!
Yes, sorry, I didn't realise the connection between the PRs, honestly I like your fix better.

I see that in your PR that you removed the keyboardShouldPersistTaps prop completely, why?

@tafelito
Copy link
Contributor Author

@kfiroo if you don't set the keyboardShouldPersistTaps, it means that it will hide the keyboard when you touch the scrollview component.

As it says it the docs
'always', the keyboard will not dismiss automatically, and the scroll view will not catch taps, but children of the scroll view can catch taps

So it means that the Keyboard will not be dismissed if you tap the list. INHO, I don't think its a good idea.

So pretty much, If you don't set the value (or you set it as 'never'), it will hide the keyboard when you tap the list, but not when you send the a message.

@kfiroo
Copy link
Collaborator

kfiroo commented Jan 13, 2017

@tafelito Whatsapp has 2 different behaviours for android and ios.
Android looks like its is set to 'never' - you can scroll the list and tap on it and the keyboard stays.
iOS will leave the keyboard open when scrolling but closes it when you tap the list.

I think it's bet to leave this decision to the users of this package
What do you think?

@tafelito
Copy link
Contributor Author

@kfiroo So I guess in that case yes, I agree with you, we can leave a default but allow the user to set a different value if they want. To me the iOS behavior looks more appropriate, but as long as the user can change the default value, I think either of them it should be ok

@tafelito
Copy link
Contributor Author

@kfiroo I'm closing this PR and creating a new one based on what we discussed earlier. And without any RN version change. At least not in this PR.

@tafelito tafelito closed this Jan 16, 2017
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.

3 participants