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

iOS - Log in - Email input is not scrollable #2726

Closed
isagoico opened this issue May 6, 2021 · 16 comments
Closed

iOS - Log in - Email input is not scrollable #2726

isagoico opened this issue May 6, 2021 · 16 comments
Assignees
Labels
Daily KSv2 External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement.

Comments

@isagoico
Copy link

isagoico commented May 6, 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:

Email input is scrollable when there's a long email

Actual Result:

Email input is not scrollable

Action Performed:

  1. Open expensify chat app
  2. On the log in screen, enter a long email
  3. Try to scroll the email and edit.

Workaround:

No workaround found for editing the email. Just deleting it and rewriting

Platform:

Where is this issue occurring?

Web
iOS ✔️
Android
Desktop App
Mobile Web

Version Number: 1.0.39-0

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

Notes/Photos/Videos:

Bug5056652_06052021_Ipad.mp4

Expensify/Expensify Issue URL:

View all open jobs on Upwork

Upwork job posting: https://www.upwork.com/jobs/~018d8909d10240a267

@isagoico isagoico added AutoAssignerTriage Auto assign issues for triage to an available triage team member Daily KSv2 labels May 6, 2021
@MelvinBot
Copy link

Triggered auto assignment to @davidcardoza (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@MelvinBot MelvinBot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label May 6, 2021
@davidcardoza
Copy link
Contributor

Confirmed I cannot scroll into the text to edit, I am only provided the ability to delete the full-text chain or continue adding to it.
IMG_6913BC3590E0-1

@davidcardoza
Copy link
Contributor

This is a good candidate to send off to upwork.

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

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

@davidcardoza davidcardoza added the Improvement Item broken or needs improvement. label May 7, 2021
@davidcardoza davidcardoza removed their assignment May 7, 2021
@isagoico
Copy link
Author

isagoico commented May 9, 2021

Issue reproducible during today's KI retests

@arielgreen
Copy link
Contributor

Posted to Upwork here.

@MelvinBot
Copy link

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

@bekazandukeli
Copy link

bekazandukeli commented May 10, 2021

Hello, I already submitted proposal on upwork and here's my workaround:
Problem seems like is rooted deeply into react-native's core as described in this Issue

I already found style property which provokes this react native bug and will fix it in no time.
I will remove this buggy property and create the custom input component which will have the exact styling without provoking any bug.

I read your contribution guidelines and my pull request will be made according to it.

Here is my quick fix video:

quick-fix.1.mp4

@roryabraham
Copy link
Contributor

@bekazandukeli Thanks for providing a video of your solution! Did you just remove the fontFamily and fontSize styles from styles.textInput? Because while it seems to fix the scrolling bug, really we would prefer to fix the root issue rather than have to compromise the styling of the text in any one-line TextInput components throughout our app. If it's just the fontSize and not the fontFamily I sort of think it might be okay, but I could use a second opinion on whether this is an acceptable solution for the time being or if we should try to dig more into the root cause in RN. cc @shawnborton

@Naiem1808015
Copy link

Naiem1808015 commented May 10, 2021 via email

@bekazandukeli
Copy link

bekazandukeli commented May 11, 2021

@bekazandukeli Thanks for providing a video of your solution! Did you just remove the fontFamily and fontSize styles from styles.textInput? Because while it seems to fix the scrolling bug, really we would prefer to fix the root issue rather than have to compromise the styling of the text in any one-line TextInput components throughout our app. If it's just the fontSize and not the fontFamily I sort of think it might be okay, but I could use a second opinion on whether this is an acceptable solution for the time being or if we should try to dig more into the root cause in RN. cc @shawnborton

Investigation

My solution is simpler, I think we won't need to create custom component either. I further investigated problem and it occurs when ratio of height / fontSize is around same value as current 40 / 15.

For example setting height: 41 fontSize: 15 fixes bug, as well as height: 40 fontSize: 14, but when height: 41 fontSize: 16 bug occurs again.

also:

39 / 14 not fixes
37 / 12 not fixes
36 / 11 fixes

Solution 🚀

I think the most short and elegant solution will be to break this magic buggy 40 / 15 ratio and make it 41 / 15 with setting height: variables.componentSizeNormal + 1, it will have no impact on whole app UI and TextInput UI change will not be noticeable for eye also. I think it's the safest fix.

P.S.

  • Removing height: variables.componentSizeNormal is a bad idea because it will cause different input size on Android / iOS since defaults on these platforms are different.
  • Creating a new custom InputField will be an overkill.
  • Changing fontSize is not the best option, since font becomes noticeably smaller/larger.

@vmv94
Copy link

vmv94 commented May 11, 2021

https://www.upwork.com/ab/proposals/1391994416817717249?success
Hello! I have a variant for fix this task. I was send you a proposal. I create a new component with scrollview out of TextInput with special styles and methods. How it work you can see on my video.

appVideo.mp4

A second solution can be TextInput with multiple strings and dynamic height.

I will glad to work with your team!

@shawnborton
Copy link
Contributor

@roryabraham do we have a clear idea of why this bug is happening? It seems like there are a variety of interesting workarounds, but I'm curious if we understand the root cause.

Ideally we keep our font size at the standard 15, and our standard input size is at 40.

@roryabraham
Copy link
Contributor

do we have a clear idea of why this bug is happening?

Short answer: no, not really 😅 It's a bug with RN itself.

Thanks for the detailed explanation @bekazandukeli. I personally would rather have your simple workaround for the time being than a custom-implemented TextInput like @vmv94 has proposed. Both solutions work, but again, the best solution would be to fix this bug upstream in RN so we don't need a workaround.

@isagoico
Copy link
Author

Issue reproducible during today's KI retests

@arielgreen
Copy link
Contributor

Thanks all. Closing this issue out — we've decided to change the scope (and raise the price).

cc @bekazandukeli @vmv94 @Naiem1808015 I encourage you to take a look at #2973 and submit a proposal if you're still interested! New Upwork job is here.

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

No branches or pull requests

9 participants