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

[$2000] Unnecessary new lines created when user mentions a contact #21615

Closed
1 of 6 tasks
kavimuru opened this issue Jun 26, 2023 · 105 comments
Closed
1 of 6 tasks

[$2000] Unnecessary new lines created when user mentions a contact #21615

kavimuru opened this issue Jun 26, 2023 · 105 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor

Comments

@kavimuru
Copy link

kavimuru commented Jun 26, 2023

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Action Performed:

  1. Open ND on Android
  2. Open any chat and type @
  3. Select any one contact from the list which has a long email
  4. Observe the mentioned contact email in the composer

Expected Result:

Extra new lines should not be created since there is enough space inside the composer

Actual Result:

Extra new lines are created and app is not using the white space after the email.

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?

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.3.32-5
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

Screen.Recording.20230626.195148.New.Expensify.mp4
Screen_Recording_20230621_161514_New.Expensify.mp4

Expensify/Expensify Issue URL:
Issue reported by: @Nathan-Mulugeta
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1687353655609189

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01c301c436377b47aa
  • Upwork Job ID: 1673794132243972096
  • Last Price Increase: 2023-07-19
  • Automatic offers:
    • s77rt | Reviewer | 27161424
    • aswin-s | Contributor | 27161425
    • nathan-mulugeta | Reporter | 27161427
@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jun 26, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 26, 2023

Triggered auto assignment to @joekaufmanexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@melvin-bot
Copy link

melvin-bot bot commented Jun 26, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@joekaufmanexpensify
Copy link
Contributor

I can reproduce this, but am not 100% convinced this is a bug. The mention does break into multiple lines with extra space. But then if you type on one of the middle lines, it fills out the extra space.

2023-06-26_16-28-45.mp4

@joekaufmanexpensify
Copy link
Contributor

Discussing here

@joekaufmanexpensify
Copy link
Contributor

Discussed here, and this is a bug, since it only happens with mentions.

@joekaufmanexpensify joekaufmanexpensify added the External Added to denote the issue can be worked on by a contributor label Jun 27, 2023
@melvin-bot melvin-bot bot changed the title Unnecessary new lines created when user mentions a contact [$1000] Unnecessary new lines created when user mentions a contact Jun 27, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 27, 2023

Job added to Upwork: https://www.upwork.com/jobs/~01c301c436377b47aa

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jun 27, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 27, 2023

Current assignee @joekaufmanexpensify is eligible for the External assigner, not assigning anyone new.

@melvin-bot
Copy link

melvin-bot bot commented Jun 27, 2023

Triggered auto assignment to Contributor-plus team member for initial proposal review - @s77rt (External)

@joekaufmanexpensify
Copy link
Contributor

Waiting for proposals

@joekaufmanexpensify
Copy link
Contributor

Same

@joekaufmanexpensify
Copy link
Contributor

Still waiting for proposals.

@melvin-bot melvin-bot bot added the Overdue label Jul 3, 2023
@joekaufmanexpensify
Copy link
Contributor

Same

@melvin-bot melvin-bot bot removed the Overdue label Jul 3, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 4, 2023

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@joekaufmanexpensify joekaufmanexpensify changed the title [$1000] Unnecessary new lines created when user mentions a contact [$2000] Unnecessary new lines created when user mentions a contact Jul 5, 2023
@melvin-bot melvin-bot bot added the Overdue label Jul 5, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 5, 2023

Upwork job price has been updated to $2000

@joekaufmanexpensify
Copy link
Contributor

Bumped price as there have been no proposals here thus far. Also just highlighted this in Slack.

@melvin-bot melvin-bot bot removed the Overdue label Jul 5, 2023
@joekaufmanexpensify
Copy link
Contributor

Still pending proposals.

@aswin-s
Copy link
Contributor

aswin-s commented Jul 7, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.

Text in composer wraps early even when there is enough white space available towards right

What is the root cause of that problem?

Native TextInput component on Android uses 'highQuality' line break strategy. This strategy scans the entire paragraph and wraps the multi line text for better readability. That's the reason it breaks the email '[email protected]' early when it encounters a series of number after the text. This behaviour is also annoying when user tries to delete characters from the end of the long string as the text wrapping keeps on changing with each character being removed.

What changes do you think we should make in order to solve the problem?

The BaseTextInput component extends the RNTextInput which is basically a wrapper over TextInput component from ReactNative. We need to set the prop `textBreakStrategy' as 'simple' to this 'RNTextInout' component, so that all instances of TextInput used within the app will have simple break strategy. This is an Android only prop which maps to the native lineBreakStrategy prop on Android.

Make change at the below line

allowFontScaling={false}

function RNTextInput(props) {
    return (
        <AnimatedTextInput
            allowFontScaling={false}
            // Add this prop 
            textBreakStrategy="simple"
            ...
/>

Before

image

After

image

What alternative solutions did you explore? (Optional)

None

@s77rt
Copy link
Contributor

s77rt commented Jul 7, 2023

@aswin-s Thanks for the proposal. Although your RCA makes sense I'm not sure we are at the very root cause yet. textBreakStrategy default value should already be simple1, how come it's not the case?

Footnotes

  1. https://github.com/Expensify/react-native/blob/9837731f7bbf93949344c3cc74fdc304764085e4/Libraries/Components/TextInput/AndroidTextInputNativeComponent.js#L196-L199

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 12, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 12, 2023

📣 @s77rt 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

@melvin-bot
Copy link

melvin-bot bot commented Oct 12, 2023

📣 @aswin-s 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@melvin-bot
Copy link

melvin-bot bot commented Oct 12, 2023

📣 @Nathan-Mulugeta 🎉 An offer has been automatically sent to your Upwork account for the Reporter role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

@aswin-s
Copy link
Contributor

aswin-s commented Oct 12, 2023

Accepted the offer and raised PR.

@joekaufmanexpensify joekaufmanexpensify changed the title [hold for upstream PR] [$2000] Unnecessary new lines created when user mentions a contact [$2000] Unnecessary new lines created when user mentions a contact Oct 16, 2023
@joekaufmanexpensify
Copy link
Contributor

Sounds good!

@aswin-s
Copy link
Contributor

aswin-s commented Oct 18, 2023

PR is deployed to production. Melvin doesn't seem to care.

@joekaufmanexpensify
Copy link
Contributor

Interesting. Yep, I see this one is all set. @s77rt @aswin-s is anything else needed for this one? If not, I will spin up the BZ checklist today manually so we can get that done, issue payment, and then close this out!

@aswin-s
Copy link
Contributor

aswin-s commented Oct 23, 2023

@joekaufmanexpensify All good from my side.

@s77rt
Copy link
Contributor

s77rt commented Oct 23, 2023

  • The PR that introduced the bug has been identified: n/a. The bug is in RN (and its outdated documents)
  • The offending PR has been commented on: n/a
  • A discussion in #expensify-bugs has been started: Not needed for this case as we already have a lint rule to force the use of our custom TextInput (which includes the fix) instead of RN's TextInput
  • Determine if we should create a regression test for this bug: I don't think this is needed for the same reason above

@s77rt
Copy link
Contributor

s77rt commented Oct 23, 2023

@joekaufmanexpensify All is good. Checklist provided above ^

@aswin-s
Copy link
Contributor

aswin-s commented Oct 23, 2023

@joekaufmanexpensify PR was merged within 3 days of assignment. Is speed bonus applicable here?

@joekaufmanexpensify
Copy link
Contributor

Switching to this to daily so I can organize the payment next steps. I will look into the applicability of urgency bonus here as well.

@joekaufmanexpensify joekaufmanexpensify added Daily KSv2 and removed Weekly KSv2 labels Oct 30, 2023
@joekaufmanexpensify
Copy link
Contributor

Discussing speed bonus.

@joekaufmanexpensify
Copy link
Contributor

joekaufmanexpensify commented Nov 1, 2023

Okay, we discussed in the above thread and confirmed this is eligible for a speed bonus. So we need to make the following payments:

  • @aswin-s - $3000 ($2000 for bounty + $1000 speed bonus). Paid via Upwork.
  • @s77rt - $3000 ($2000 for C+ review + $1000 speed bonus). Paid via Upwork.
  • @Nathan-Mulugeta - $250 for reporting. Paid via Upwork.

@joekaufmanexpensify
Copy link
Contributor

@aswin-s $3000 sent and contract ended!

@Nathan-Mulugeta
Copy link

Hey @joekaufmanexpensify The reporting bonus for this one is based on the previous bounty since this issue was opened before the adjustment

@joekaufmanexpensify
Copy link
Contributor

@s77rt $3000 sent and contract ended!

@joekaufmanexpensify
Copy link
Contributor

Hey @joekaufmanexpensify The reporting bonus for this one is based on the previous bounty since this issue was opened before the adjustment

Ah, good call. Let me double check the timing on that!

@joekaufmanexpensify
Copy link
Contributor

joekaufmanexpensify commented Nov 1, 2023

Okay, yep. Reporting will be $250, since this was reported before August, 30th.

@Nathan-Mulugeta
Copy link

Great thanks

@joekaufmanexpensify
Copy link
Contributor

@Nathan-Mulugeta $250 sent and contract ended!

@joekaufmanexpensify
Copy link
Contributor

Bug is fixed, and all payment issued. Closing as this is all set. Thanks everyone!

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

No branches or pull requests

7 participants