Closed kavimuru closed 1 year ago
Triggered auto assignment to @conorpendergrast (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
Platforms
in OP are β
)Reproduced on OSX desktop app too
Job added to Upwork: https://www.upwork.com/jobs/~012eff16d4d91d5c21
Current assignee @conorpendergrast is eligible for the External assigner, not assigning anyone new.
Triggered auto assignment to Contributor-plus team member for initial proposal review - @aimane-chnaif (External
)
After going back from another chat, the composer won't focus after pressing Mark as read/unread. To be clearer, the composer focus manager doesn't work.
We have a composer focus manager that will listen to any call of ReportActionComposeFocusManager.focus()
.
https://github.com/Expensify/App/blob/94e265653f83505e581e99ac0ec0d3ea30e23998/src/pages/home/report/ReportActionCompose.js#L229-L237
When we open chat A, a listener is added. https://github.com/Expensify/App/blob/94e265653f83505e581e99ac0ec0d3ea30e23998/src/libs/ReportActionComposeFocusManager.js#L7-L15
Navigating to chat B will replace the A listener with B. When we press back from B, B will unmount and the listener is cleared. https://github.com/Expensify/App/blob/94e265653f83505e581e99ac0ec0d3ea30e23998/src/pages/home/report/ReportActionCompose.js#L275-L277
Initialize the listener on both mount and update. On update, only add the listener if the screen becomes focused, i.e.
if (!prevProps.isFocused && this.props.isFocused) { focusManager.onComposerFocus() }
Currently, we only store the latest callback. Instead,
clear
.focus
, iterate over the array and invoke the callbackLooks like something related to react-navigation
may have been mentioned in this issue discussion.
As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js
files should not be accepted.
Feel free to drop a note in #expensify-open-source with any questions.
Proposals are in review
π£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πΈ
@aimane-chnaif Do any of these proposals look promising?
Still evaluating
@aimane-chnaif Still nothing?
@conorpendergrast @aimane-chnaif this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!
π£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πΈ
@bernhardoj I agree with your RCA. Can you make sure that your solution doesn't break current autofocus behavior on mobile? Especially when report screens are stacked and click back button?
I currently have issues running the app. Will confirm once it's resolved!
current autofocus behavior on mobile
this still works fine
report screens are stacked and click back button
So, the ReportActionComposeFocusManager.onComposerFocus
is actually for the web and is only triggered from context menu actions, so this one doesn't change anything related to the autofocus. Also, autofocus after going back from a report is also implemented only for the web, but I still tested it and it works fine.
willBlurTextInputOnTapOutside
is false for the native.
@bernhardoj what about mWeb?
Tested on mWeb too
@aimane-chnaif Looks like we're good to go here, is that correct?
@aimane-chnaif Looks like we're good to go here, is that correct?
yes, @bernhardoj's proposal looks good. π π π C+ reviewed
Triggered auto assignment to @grgia, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
@conorpendergrast @grgia @aimane-chnaif this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!
π£ @aimane-chnaif π An offer has been automatically sent to your Upwork account for the Reviewer role π Thanks for contributing to the Expensify app!
π£ @bernhardoj You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review π§βπ» Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing π
π£ @dhanashree-sawant π An offer has been automatically sent to your Upwork account for the Reporter role π Thanks for contributing to the Expensify app!
PR is ready
cc: @aimane-chnaif
π― β‘οΈ Woah @aimane-chnaif / @bernhardoj, great job pushing this forwards! β‘οΈ
The pull request got merged within 3 working days of assignment, so this job is eligible for a 50% #urgency bonus π
On to the next one π
@bernhardoj A reminder to apply to the Upwork job please!
@conorpendergrast Applied
Reviewing
label has been removed, please complete the "BugZero Checklist".
The solution for this issue has been :rocket: deployed to production :rocket: in version 1.3.45-7 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:
If no regressions arise, payment will be issued on 2023-08-02. :confetti_ball:
After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.
For reference, here are some details about the assignees on this issue:
As a reminder, here are the bonuses/penalties that should be applied for any External 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:
Payouts due:
Eligible for 50% #urgency bonus? Y
Upwork job is here
Everyone paid. @aimane-chnaif C+ checklist, please, and then I'll do a regression test (if necessary) and we can close this out
I don't think regression test is needed as this isn't critical issue and doesn't prevent user using the app. Even it's not easy to follow repro step.
Alright, then we're all done. Thank you!
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:
App should focus on compose box on click of mark as unread/read for any chat from LHN
Actual Result:
App does not focus on compose box on click of mark as unread/read for group chat from LHN if we open group details, click on any user, click message and click on browser back to come back to group chat
Workaround:
Can the user still use Expensify without this being fixed? Have you informed them of the workaround?
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.3.33-4 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation
https://github.com/Expensify/App/assets/43996225/04a8c119-789d-453f-adf9-0da4a6593b03
https://github.com/Expensify/App/assets/43996225/5899072b-fcb4-40df-928c-fb3fc7566561
Expensify/Expensify Issue URL: Issue reported by: @dhanashree-sawant Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1687606178130269
View all open jobs on GitHub
Upwork Automation - Do Not Edit