Skip to content
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

Tapping on Edit Message scrolls down to the bottom to the compose box #3545

Closed
isagoico opened this issue Jun 11, 2021 · 12 comments · Fixed by #3551
Closed

Tapping on Edit Message scrolls down to the bottom to the compose box #3545

isagoico opened this issue Jun 11, 2021 · 12 comments · Fixed by #3551
Assignees
Labels
Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor

Comments

@isagoico
Copy link

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Expected Result:

Describe what you think should've happened

Expected result:

User stays in the same position and is able to edit the message

Actual result:

Tapping on Edit scrolls down the page to the compose box

Actions Performed:

  1. Launch the app and log in
  2. Hover over a sent message and click on the edit comment option in the menu
  3. Start editing the message

Platform:

Android ✔️

Notes/Images/Video:

Bug5107427_Screen_Recording_20210610-190557_Expensifycash.mp4

Version Number: 1.0.67-0

Logs: https://stackoverflow.com/c/expensify/questions/4856

Expensify/Expensify Issue URL:

View all open jobs on Upwork

@isagoico isagoico added DeployBlockerCash This issue or pull request should block deployment Engineering Daily KSv2 labels Jun 11, 2021
@MelvinBot
Copy link

Triggered auto assignment to @francoisl (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@OSBotify
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@isagoico
Copy link
Author

Added the deploy blocker label. We didn't experience this issue on last regression.

@Julesssss
Copy link
Contributor

Hey @isabelastisser, do you know if is this Android only or also occurring on iOS?

@Julesssss Julesssss assigned Julesssss and unassigned Julesssss Jun 11, 2021
@trjExpensify
Copy link
Contributor

I can reproduce this on iOS v1.0.67-0.

Image.from.iOS.3.MP4

@francoisl
Copy link
Contributor

In a 1:1 right now, but I can take a look a bit later, otherwise anyone feel free to start digging and I'm happy to help later too.

@parasharrajat
Copy link
Member

parasharrajat commented Jun 11, 2021

@Julesssss
Can I propose a fix here?

The issue is this line https://github.com/Expensify/Expensify.cash/blob/3284a15cb0bef712dc44828905e26943ed5491f8/src/pages/home/report/ReportActionsView.js#L110.

But this is necessary. So we should add a check in the event listener that when the main ReportActionComposer is focused only then scrolltoBottom.

But now the question is how we will get the focused state of composer here? I don't like nested forwarding the refs. so as per my PR, we are already implementing the ReportActionComposerFocusManager. We can extend the same lib to have a function
isFocused tells us about the focused state of the Composer. Now we can use it here https://github.com/Expensify/Expensify.cash/blob/3284a15cb0bef712dc44828905e26943ed5491f8/src/pages/home/report/ReportActionsView.js#L110.
and conditionally scroll to the bottom.

Implementation of isFocused.

const composerRef = React.createRef();
function isFocsued(){
   if(composerRef.current){
     composerRef.current.isFocused();
   }
}

we can then set composerRef in ReportActionComposer.

@Julesssss
Copy link
Contributor

That sounds great to me @parasharrajat. Thanks for taking a look!

We'll also remove the deployBlocker here as it's not critical enough to block our release. Let's get an UpWork issue created for you too.

@Julesssss Julesssss added External Added to denote the issue can be worked on by a contributor and removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Jun 11, 2021
@MelvinBot
Copy link

Triggered auto assignment to @MitchExpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@parasharrajat
Copy link
Member

No worries, But is PR is up here #3551. Thanks

@isagoico
Copy link
Author

Issue reproducible today during KI retests

@MitchExpensify
Copy link
Contributor

Job created here and have invited you @parasharrajat !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants