-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[HOLD for payment 2022-12-15] [$500] Login - Clicking the eye icon moves the cursor to the beginning #12018
Comments
Triggered auto assignment to @Julesssss ( |
Yep, I can reproduce. @kbecciv would you mind re-uploading a video? I think you uploaded a different bug, so I removed it. |
Triggered auto assignment to @flaviadefaria ( |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @eVoloshchak ( |
Current assignee @Julesssss is eligible for the External assigner, not assigning anyone new. |
I don't think it's worthy enough to fix. |
Oh, thanks for linking! While I agree with most of that discussion we've recently decided to try fixing each and every bug we encounter. For that reason, I think we should reopen this and see what sort of proposals we receive. If the solution is very convoluted, then maybe it would be preferable to just default to the end, given that mobile and some web browsers do this by default. |
@Julesssss Video is attached, sorry for missing it. |
Thanks, no worries |
@eVoloshchak, @Julesssss, @flaviadefaria Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
1 similar comment
@eVoloshchak, @Julesssss, @flaviadefaria Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
Awaiting proposals |
link to the internal job posting: posting https://www.upwork.com/ab/applicants/1584804132817670144/job-details |
Thanks for the proposal. But I'm curious whether there's a better solution that doesn't require us to manually set the cursor. |
Apparently the cursor resetting is just react-native internal behaviour, I tested on snack and got the cursor going to the beginning as well. I tried to fiddle around with other textInput components which was unsuccessful but I think that would be unnecessary and just add extra code. This was the solution that was the simplest and most importantly added the least amount of code. And is much more reliable than relying on a third-party component. |
Seems like this introduced this regression, so I'm going to have to revert it. |
I'm really sorry for this. I'll fix it asap |
No worries. I also missed this. |
I found out a solution. Should I make a new PR? |
Yeah, just give me a few minutes to revert the previous changes to unblock our staging build. It'll be a few more minutes, and then you can raise the PR against a clean |
@Someone-here all done, no rush with the new PR as I won't be able to review until next week |
Okay, thank you for giving me another chance. I will do extensive testing before submitting PR this time. |
Made a new PR #13317 This fixes the regression. I have done extensive testing with checkboxes on all platforms as well. I hope this serves its purpose this time... |
Heads up I'm heading OoO for a week so in case this needs to be paid out before I come back then please reassign or ask someone else in the team to process the payment. Thanks! |
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.36-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2022-12-15. 🎊 After the hold period, please check if any of the following need payment for this issue, and if so check them off after paying:
As a reminder, here are the bonuses/penalties that should be applied for any External issue:
|
I decided to close the outstanding PR on the second review as it's adding quite a bit of complexity to the Checkbox component. Explained a bit more here. @flaviadefaria could you please pay out the original contract when you're back? We had a regression so I believe it will be 50% of the payment |
It also was merged within ~12 days, which is a 50% penalty (or contract termination?) I don't think I'm eligible for any payment on this one, there were a couple of regressions I could have noticed, we've dragged this out for more than 2 weeks, I don't want to get paid for this one |
Fair enough, thanks @eVoloshchak but you still reviewed the PR originally so I think the reduced payment is fine. Honestly, I'm not sure the 12-day rate counts here due to the regression. |
Guys, I am really sorry if you think I did not do upto to the mark but I tried my best.... |
Hey @Someone-here don't worry. I should have called it out earlier in the process and maybe we could have discussed alternatives. Let's just move onto the next issue 👍 |
Great! Thank you for the positive attitude |
We'll be paying out $250 for each of you (50% penalty). I've sent you both the contract offer in UW please accept and let me know here once done. Thanks! |
Everyone has been paid so closing this out. |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Action Performed:
Expected Result:
The cursor should not moved
Actual Result:
The cursor moved to the beginning
Workaround:
Unknown
Platform:
Where is this issue occurring?
Version Number: 1.2.18.2
Reproducible in staging?: Yes
Reproducible in production?: Yes
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
video_2022-10-20_12-37-43.mp4
@Julesssss removed the capture as it was an unrelated recording
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:
View all open jobs on GitHub
The text was updated successfully, but these errors were encountered: