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

Slightly scrolling up in compose box will dismiss the keyboard #3323

Closed
isagoico opened this issue Jun 2, 2021 · 12 comments · Fixed by #3733
Closed

Slightly scrolling up in compose box will dismiss the keyboard #3323

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

Comments

@isagoico
Copy link

isagoico commented Jun 2, 2021

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:

Scrolling up in a long message should not dismiss the keyboard.

Actual Result:

Slightly scrolling up the message in compose box will dismiss the keyboard.

Action Performed:

  1. Log in on mobile app
  2. Navigate to a conversation
  3. Tap on compose box and write a long message (long enough to enable scrolling)
  4. Scroll the message to the top.

Workaround:

User has to recall the keyboard everytime after scrolling up.

Platform:

Where is this issue occurring?

Web
iOS ✔️
Android ✔️
Desktop App
Mobile Web

Version Number: 1.0.61-0

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

Notes/Photos/Videos: Any additional supporting documentation

nosound.mp4

Expensify/Expensify Issue URL:

View all open jobs on Upwork

@MelvinBot
Copy link

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

@bondydaa
Copy link
Contributor

bondydaa commented Jun 2, 2021

tossing to the pool

@bondydaa bondydaa removed their assignment Jun 2, 2021
@isagoico
Copy link
Author

isagoico commented Jun 7, 2021

Issue reproducible today during KI retests

@arielgreen arielgreen added the External Added to denote the issue can be worked on by a contributor label Jun 7, 2021
@MelvinBot
Copy link

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

@isagoico
Copy link
Author

Issue reproducible today during KI retests

@kevinksullivan
Copy link
Contributor

kevinksullivan commented Jun 19, 2021

Sorry for the delay on this one. Posted to upwork!

https://www.upwork.com/jobs/~01b905fa59a688b29f

@MelvinBot
Copy link

Triggered auto assignment to @puneetlath (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@parasharrajat
Copy link
Member

parasharrajat commented Jun 19, 2021

Proposal

  1. We are using PanRasponder over the Composer to close it but it lacks two basic checks.
  2. First we should only close the keyboard when the user is swiping down.
  3. Second, We need to increase the swipe distance, as optional we can also check for the velocity of the swipe so that if a user is swiping fast , only then close it. (Let me know about this optional part).
    To fix the above problem at base we need to do the following update to code here https://github.com/Expensify/Expensify.cash/blob/408b7e00cf33c16600af0b67b819c764fcb5388a/src/components/SwipeableView/index.native.js#L16
// We increase the distance, we need to carefully adjust this value but I suggest it should be equal to the max height of composer
        const minimumPixelDistance = 116;
        this.oldY = 0;
        this.panResponder = PanResponder.create({

            // The PanResponder gets focus only when the y-axis movement is over minimumPixelDistance
            onMoveShouldSetPanResponderCapture:
            (_event, gestureState) => {
                // we only close it when direction is bottom and minimumdistance is covered.
                if ((gestureState.dy - this.oldY) > 0 && gestureState.dy > minimumPixelDistance) { return true; }
                this.oldY = gestureState.dy;
            },

            // Calls the callback when the swipe down is released; after the completion of the gesture
            onPanResponderRelease: this.props.onSwipeDown,
        });
    }
    ```

@isagoico
Copy link
Author

Issue reproducible today during KI retests.

@puneetlath
Copy link
Contributor

I say we just try 2 first and see how that feels. Then we can add 3 in a follow up issue if we think it's also necessary.

@kevinksullivan
Copy link
Contributor

Hired @parasharrajat !

@kevinksullivan
Copy link
Contributor

Paid in upwork

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.

7 participants