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

[HOLD for payment 2024-07-10] [$250] Login - "Chat and split expenses" option is truncated on the onboarding modal #43804

Closed
1 of 6 tasks
m-natarajan opened this issue Jun 15, 2024 · 30 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@m-natarajan
Copy link

m-natarajan commented Jun 15, 2024

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


issue found when executing #43479
Version Number: 1.4.84-0
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
Expensify/Expensify Issue URL:
Issue reported by: applause internal team
Slack conversation:

Action Performed:

  1. Open the app
  2. Log in with a new Gmail account

Expected Result:

"Chat and split expenses" option should be fully visible.

Actual Result:

"Chat and split expenses" option is truncated on the onboarding modal.

Workaround:

visual

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6514288_1718458311334.az_recorder_20240615_151800.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01c02ce36ab64ecdf6
  • Upwork Job ID: 1804256617422060262
  • Last Price Increase: 2024-06-21
  • Automatic offers:
    • allgandalf | Reviewer | 102851828
    • nkdengineer | Contributor | 102851830
Issue OwnerCurrent Issue Owner: @OfstadC
@m-natarajan m-natarajan added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jun 15, 2024
Copy link

melvin-bot bot commented Jun 15, 2024

Triggered auto assignment to @OfstadC (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@m-natarajan
Copy link
Author

@OfstadC FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

@neonbhai
Copy link
Contributor

neonbhai commented Jun 15, 2024

Proposal

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

"Chat and split expenses" option is truncated on the onboarding modal

What is the root cause of that problem?

The number of lines for menuitem titles is set to 1 by default

numberOfLinesTitle = 1,

This restricts the onboarding titles to only 1 line in narrow screens

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

We will add the numberOfLines style for onboarding choices as 2 to make sure text gets rendered properly for smaller devices

We will add numberOfLinesTitle: 2 for onboarding menuItems here:

return {
key: translationKey,
title: translate(translationKey),

Alternatively

We can pass this style by adding a prop in MenuItemsList here

Result:

Before Screenshot 2024-06-18 at 7 37 53 PM
After Screenshot 2024-06-18 at 7 39 05 PM

@OfstadC
Copy link
Contributor

OfstadC commented Jun 17, 2024

I'm not able to reproduce 🤔
2024-06-17_15-04-37 (1)

@neonbhai
Copy link
Contributor

@OfstadC hi, this is visible for devices with smaller screens! We can also resize view on web to have it show up

Screenshot 2024-06-18 at 7 37 53 PM

@OfstadC
Copy link
Contributor

OfstadC commented Jun 18, 2024

Okay - I tried on about 6 android devices, 3 of which google said had smaller screens and the only one I was able to reproduce on was Samsung S24 😅 .
image

@Julesssss Julesssss self-assigned this Jun 19, 2024
@OfstadC
Copy link
Contributor

OfstadC commented Jun 20, 2024

Hi @Julesssss ! New to BZ here so just want to make sure I take the next steps. I see you self assigned, so should I add the internal label - or do we still want to create an external posting?

@Julesssss
Copy link
Contributor

Hey @OfstadC, I assigned just because I was interested in seeing the solution to the bug. So we can follow the normal process 🙂

@OfstadC
Copy link
Contributor

OfstadC commented Jun 21, 2024

Discussed in Slack - The solution should ensure we aren't truncating when viewing or selecting the menu item. We'd want to let any device/width use as many lines as needed

@OfstadC OfstadC added the External Added to denote the issue can be worked on by a contributor label Jun 21, 2024
@melvin-bot melvin-bot bot changed the title Login - "Chat and split expenses" option is truncated on the onboarding modal [$250] Login - "Chat and split expenses" option is truncated on the onboarding modal Jun 21, 2024
Copy link

melvin-bot bot commented Jun 21, 2024

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

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

melvin-bot bot commented Jun 21, 2024

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

@nkdengineer
Copy link
Contributor

nkdengineer commented Jun 22, 2024

Proposal

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

"Chat and split expenses" option is truncated on the onboarding modal.

What is the root cause of that problem?

By default, the title of MenuItem will have 1 as numberOfLinesTitle

numberOfLinesTitle = 1,
.

Therefore, when the text is long such that it should span 2 lines, it will be truncated.

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

The solution should ensure we aren't truncating when viewing or selecting the menu item. We'd want to let any device/width use as many lines as needed

This is the requirement. We need to make sure the numberOfLines here is not set. One way to do it is to add numberOfLinesTitle: 0 here so no truncation will occur in any circumstance.

We use the same method in other locations like in

where no-truncation behavior is needed.

FYI any hardcoded value like 2 will not work reliably because there could be very small devices that will require 3 lines to display the item, or the copy of the title could be changed such that it needs more than 2 lines in smaller devices.

What alternative solutions did you explore? (Optional)

Set the default numberOfLinesTitle here to null/undefined/0 (and maybe the numberOfLinesDescription too). Truncating texts should be a conscious decision and should only be done intentionally, not accidentally just because the default value is set that way.

@allgandalf
Copy link
Contributor

Discussed in Slack - The solution should ensure we aren't truncating when viewing or selecting the menu item. We'd want to let any device/width use as many lines as needed

Considering the expected result, lets go with @nkdengineer's proposal here as it is more future proof

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Jun 22, 2024

Current assignee @Julesssss is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

@Julesssss
Copy link
Contributor

SOlution is simple and makes sense

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jun 24, 2024
Copy link

melvin-bot bot commented Jun 24, 2024

📣 @allgandalf 🎉 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

Copy link

melvin-bot bot commented Jun 24, 2024

📣 @nkdengineer 🎉 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 📖

@allgandalf
Copy link
Contributor

@nkdengineer , let us know when can we expect a PR

@nkdengineer
Copy link
Contributor

I'll raise PR soon

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Jun 26, 2024
@nkdengineer
Copy link
Contributor

@allgandalf this PR is ready for review

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Jul 3, 2024
@melvin-bot melvin-bot bot changed the title [$250] Login - "Chat and split expenses" option is truncated on the onboarding modal [HOLD for payment 2024-07-10] [$250] Login - "Chat and split expenses" option is truncated on the onboarding modal Jul 3, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Jul 3, 2024
Copy link

melvin-bot bot commented Jul 3, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Jul 3, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.3-7 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-07-10. 🎊

For reference, here are some details about the assignees on this issue:

Copy link

melvin-bot bot commented Jul 3, 2024

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@allgandalf] The PR that introduced the bug has been identified. Link to the PR:
  • [@allgandalf] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@allgandalf] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@allgandalf] Determine if we should create a regression test for this bug.
  • [@allgandalf] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@OfstadC] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: https://github.com/Expensify/Expensify/issues/411753

@OfstadC
Copy link
Contributor

OfstadC commented Jul 9, 2024

@allgandalf Could you please update this today based on the checklist? Thank you! 😃

@allgandalf
Copy link
Contributor

yes i will complete the checklist in sometime 👍

@allgandalf
Copy link
Contributor

  • The PR that introduced the bug has been identified. Link to the PR: [Onboarding - Stage 1] Onboarding Flow #37733

  • The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: https://github.com/Expensify/App/pull/37733/files#r1672199582

  • A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A

  • If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Proposal

  1. Open the app on MacOS web
  2. Log in with a new Gmail account
  3. Get into responsive mode and change the size of the screen to different length and width.

Verify that: "Chat and split expenses" option isn't truncated

Do we agree 👍 or 👎

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jul 10, 2024
@OfstadC
Copy link
Contributor

OfstadC commented Jul 10, 2024

Payment Summary:

Contributor+: @allgandalf paid $250 via Upwork
Contributor: @nkdengineer paid $250 via Upwork

@OfstadC
Copy link
Contributor

OfstadC commented Jul 11, 2024

Leaving this open until regression testing is complete

@melvin-bot melvin-bot bot added the Overdue label Jul 15, 2024
@OfstadC
Copy link
Contributor

OfstadC commented Jul 15, 2024

Same - waiting on regression testing

@melvin-bot melvin-bot bot removed the Overdue label Jul 15, 2024
@OfstadC
Copy link
Contributor

OfstadC commented Jul 16, 2024

I'm actually going to close this out. but I'll note here if the regression test yields anything 😄

@OfstadC OfstadC closed this as completed Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
Archived in project
Development

No branches or pull requests

6 participants