Open bernhardoj opened 1 week ago
@jayeshmangwani Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and not onIconClick
).src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components using Avatar
have been tested & I retested again)/** comment above it */
this
properly so there are no scoping issues (i.e. for onClick={this.submit}
the method this.submit
should be bound to this
in the constructor)this
are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this);
if this.submit
is never passed to a component event handler like onClick
)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG
)Avatar
is modified, I verified that Avatar
is working as expected in all cases)Design
label and/or tagged @Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test
steps.@bernhardoj, can we please update the QA step and add a note that after this PR, we are updating all cases where the composer for mobile devices should not be auto-focused? I believe some TestRail tests expect the input to auto-focus on mobile devices, which may lead to deployment blockers.
Done.
Btw, can we hold until https://github.com/Expensify/App/issues/56358 is solved? That issue is only reproducible when the composer is refocused after sending a password-protected PDF. This PR hasn't fixed the refocus behavior when sending attachment yet. After that issue is fixed, I'll try to fix the refocus behavior after sending an attachment.
While waiting for the other PR, I tried to find the root cause of why the composer was focused after sending an attachment. This is because we focus on the composer every time the app comes from the background.
In our case, when selecting an attachment, the app goes to the background, and when we back to the app, the app goes to the foreground (active) again.
Do we want to keep this behavior?
https://github.com/user-attachments/assets/f72567dc-a470-490b-8d8d-5b964e56b734
Do we want to keep this behavior?
IMHO, composer shouldn't be the focus, but I'll defer to @shawnborton and @marcochavezf.
While waiting for the other PR
Thanks for the update! I'll re-review this PR once the other PR is merged.
Agree, we shouldn't auto focus for the user there.
Explanation of Change
Fixed Issues
$ https://github.com/Expensify/App/issues/55485 PROPOSAL: https://github.com/Expensify/App/issues/55485#issuecomment-2603817017
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
Prerequisite: have a workspace chat with unsettled expense
NOTE: This PR changes the autofocus behavior of composer. Mobile screen won't have autofocus anymore.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
https://github.com/user-attachments/assets/4f4d3531-04b8-4df3-bcfd-d4b9c852a4f7Android: mWeb Chrome
https://github.com/user-attachments/assets/f26c2ca4-1c77-4553-991d-e79240690f8biOS: Native
https://github.com/user-attachments/assets/99cfad69-062f-4d14-b0e3-bc00adcbc9f9iOS: mWeb Safari
https://github.com/user-attachments/assets/a1bc8d8a-e0bf-46ba-9582-0f14c229a049MacOS: Chrome / Safari
https://github.com/user-attachments/assets/194b5473-6c83-46e5-9253-f0460f80f312MacOS: Desktop
https://github.com/user-attachments/assets/d31375a1-e0ee-47c7-b291-fc36bca092b4