-
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 2024-10-22] [$2000] Android/IOS - Split/request money - Chat composer is focused after IOU request from green plus button #10731
Comments
Triggered auto assignment to @neil-marcellini ( |
Hmm, so if you navigate to any normal chat on mobile, the composer is not focused until you tap into it. That is the desired behavior. I think this is a valid issue if we change it from: Also it looks like this only happens when the request is with a brand new chat. I was not able to see the issue when sending a request via the green plus to a chat where there was a good amount of history and zero IOU requests. This is a good external issue with lower priority (weekly). |
I updated the issue title and description to reflect the actual problem. |
Triggered auto assignment to @bfitzexpensify ( |
Upwork job here |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel ( |
Triggered auto assignment to @iwiznia ( |
Proposal
This issue is caused by _.size(this.props.reportActions) === 1 in autofocus props of Composer when this.props.reportActions === 1 is true it focuses in the input field to fix the issue we need to remove it. Solution :- After Fixing :- Untitled.mov |
ProposalProblemThe issue is in This is due to when we split/request money for the first time for a new group/person, the The previous proposal to remove the SolutionWe'll implement a new +isNewChat() {
+ if (_.size(this.props.reportActions) === 1) {
+ const firstReportAction = _.head(_.values(this.props.reportActions));
+ return firstReportAction.actionName === CONST.REPORT.ACTIONS.TYPE.CREATED;
+ }
+ return false;
+ }
....
<Composer
- autoFocus={this.shouldFocusInputOnScreenFocus || _.size(this.props.reportActions) === 1}
+ autoFocus={this.shouldFocusInputOnScreenFocus || this.isNewChat()} After the fix, the chat composer is no longer focused incorrectly Simulator.Screen.Recording.-.iPhone.13.-.2022-09-02.at.17.15.09.mp4I also discover another bug where the Chat composer will initially have the The fix for that will use the same Please let me know if you have any concerns regarding the above proposal. |
@iwiznia, @bfitzexpensify, @Santhosh-Sellavel Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
As mentioned by @christianwen _.size(this.props.reportActions) === 1 will cause a regression because that code is there so that when a user enters a new chat, the keyboard will be focused automatically. But I tested it thoroughly when was proposing a solution the composer was not focused when a user enters a new chat Simulator.Screen.Recording.-.iPhone.13.Pro.-.2022-09-05.at.13.10.13.mp4So I thought there is no need of _.size(this.props.reportActions) === 1 so we will be able to resolve the issue by removing this condition. If we need to focus when a user enters a new chat then we could change condition to _.size(this.props.reportActions) === 0 or we need to find a solution to check why _.size(this.props.reportActions) === 1 is not getting correct size of messages on intial render. Solution after _.size(this.props.reportActions) === 0 :-Simulator.Screen.Recording.-.iPhone.13.Pro.-.2022-09-05.at.13.21.58.mp4We need to identify first what we want to achieve does the composer needs to be focused on a new chat or not ? After identifying what needs to be done then we can work on a solution. @neil-marcellini |
@syedsaroshfarrukhdot thanks for investigating. I think we want to have the composer focused when creating a new chat but not after creating an IOU request. I'll let @iwiznia handle this going forward since he's the CME on this issue. |
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results. If a regression has occurred and you are the assigned CM follow the instructions here. If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future. |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.48-2 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 2024-10-22. 🎊 For reference, here are some details about the assignees on this issue:
|
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
I'll try to investigate the regression and open the PR soon |
Created the draft PR. Will test more case before opening it for review |
This is a super old GH so please let me know if I'm missing anything (as I was just added) I know we're working on a regression, but I'm adding this payment to remind me about it. I'll update this as needed for the final payment.
|
@Christinadobrzyn The PR still be reviewing |
Payment Summary
BugZero Checklist (@Christinadobrzyn)
|
Gonna move this back to weekly while we wait for the regression PR to go into production and have a new payment date. |
I faced with some issues when addressing the regressions. I'll open it in 1-2 days |
The PR is ready for review @DylanDylann |
Note for Melvin - PR under review cc @DylanDylann |
Just checking on this @DylanDylann can you provide an update on the PR review? Thanks! |
I am reviewing the PR |
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:
Enter an amount and select the
Next
confirmation buttonComplete the request money flow
Expected Result:
Always focus the input, but prevent keyboard from opening on mobile & mWeb
Actual Result:
The composer is focused.
Workaround:
Swipe the keyboard down to un-focus the composer
Platform:
Where is this issue occurring?
Version Number: 1.1.95.4
Reproducible in staging?: Yes
Reproducible in production?: Yes
Email or phone of affected tester (no customers): any
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Bug5712230_Record_2022-08-31-20-06-38_4f9154176b47c00da84e32064abf1c48.mp4
Bug5712230_Record_2022-08-31-20-24-05_4f9154176b47c00da84e32064abf1c48.mp4
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:
View all open jobs on GitHub
Issue Owner
Current Issue Owner: @ChristinadobrzynThe text was updated successfully, but these errors were encountered: