-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Removes onTransitionEnd callback #5965
Removes onTransitionEnd callback #5965
Conversation
@AlfredoAlc this is a small thing, but would you change the steps in the description to indicate what we want to see? Also it's nice to include a link to the site in step one for our QA testers. For example:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't look like the paypalUsernameInputRef
is used anywhere else, so you can take it out of the constructor and the ExpensiTextInput.
Went to go see what's going on with the iOS test, looks like we have some versioning issues. |
Not gonna merge this just yet. There's a discussion about the iOS tests here if you'd like to follow along: https://expensify.slack.com/archives/C01GTK53T8Q/p1634754414008400 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is going against the issue and the proposal. It should place the cursor at the end instead of removing the autofocus.
cc: @Luke9389
@parasharrajat It was later discussed that it would be better to remove the autofocus. |
Thanks. Didn't see the other proposal. |
I've got a PR up to fix the iOS tests. Once that's merged you should be able to pull master and we can get this squared away. |
@Luke9389 Thanks for keeping me updated! |
Alright, @AlfredoAlc, you should be able to merge master and the tests will work. |
@AlfredoAlc just want to double-check (I ask all my contributors this). Have you tested this since the last code change you made? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM based on new proposal.
@Luke9389 Yes, I tested on all platforms. |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to production by @roryabraham in version: 1.1.10-2 🚀
|
Details
Removes
onTransitionEnd
callback where the focus is set toTextInput
.Fixed Issues
$ #5403
Tests
QA Steps
Tested On
Screenshots
Web
web.mov
Desktop
Desktop.mov
iOS
iOS.mp4
Android
Android.mov