[HOLD for payment 2023-11-22] [$500] Inconsistency in the request money preview text size

m-natarajan commented 1 year ago

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

Version Number: 1.3.90-1 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: @ayazhussain79 Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1698087869176119

Action Performed:

Open a chat
Go to the request money
Enter a specific amount and send the request money
Click on the requested money
Check the preview text size
Expected Result:

The request money preview text size should be the same in both the chat and the review sections

Actual Result:

There is an inconsistency in the request money preview text size

Workaround:

unknown

Platforms:

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

[x] Android: Native
[x] Android: mWeb Chrome
[x] iOS: Native
[x] iOS: mWeb Safari
[ ] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

Android: Native

https://github.com/Expensify/App/assets/38435837/6a595c70-9408-4c4a-9536-aed19774eb92

Android: mWeb Chrome

https://github.com/Expensify/App/assets/38435837/9c127523-8704-4cf5-9500-c0dac67bd9e8

iOS: Native

https://github.com/Expensify/App/assets/38435837/a4bc9422-a3ac-40f7-93da-1f66bd48281f https://github.com/Expensify/App/assets/38435837/3c0981bd-0159-48c6-ab68-0c4f48efa5f7

iOS: mWeb Safari

https://github.com/Expensify/App/assets/38435837/e5f86736-305c-4fae-be26-f88c3e603800

MacOS: Chrome / Safari

https://github.com/Expensify/App/assets/38435837/cf339ede-83d4-41f5-901c-28cfc1a2fdea

MacOS: Desktop

https://github.com/Expensify/App/assets/38435837/f2b17973-f01a-4872-8773-efd2c5a994d7

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01a260f6acb4caa392
Upwork Job ID: 1716855328239075328
Last Price Increase: 2023-10-31
Automatic offers:

fedirjh | Reviewer | 27543446
dukenv0307 | Contributor | 27543447

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

wlegolas commented 1 year ago

Proposal

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

Inconsistency in the request money preview text size

What is the root cause of that problem?

In the MoneyRequestPreview component the information has the styles applied by "moneyRequestPreviewAmount" styles, this style doesn't have the font-size and line-height values has the text in the ReportPreview has.

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

We should change the text in the MoneyRequestPreview component to use the style "textHeadline" instead of "moneyRequestPreviewAmount", and remove the fontSize that is calculate for small devices.

What alternative solutions did you explore? (Optional)

N/A

POC

https://github.com/Expensify/App/assets/698363/657f4c60-babc-4246-9edf-14a8b085fb40

dukenv0307 commented 1 year ago

Proposal

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

Inconsistency in the request money preview text size

What is the root cause of that problem?

The amount text of ReportPreview has style styles.textHeadline which has the constant fontSize style

https://github.com/Expensify/App/blob/b72dfd4443022cab7a0d53e855b55144e61017e9/src/components/ReportActionItem/ReportPreview.js#L221

But in MoneyRequestPreivew the amount text has the fontSize based on the screen width

https://github.com/Expensify/App/blob/b72dfd4443022cab7a0d53e855b55144e61017e9/src/components/ReportActionItem/MoneyRequestPreview.js#L279

https://github.com/Expensify/App/blob/b72dfd4443022cab7a0d53e855b55144e61017e9/src/styles/StyleUtils.ts#L1263

That makes the inconsistency font size of the amount text in small screen width

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

In ReportPreview we should get the fontSize of amount text in the same way we do in MoneyRequestPreivew

https://github.com/Expensify/App/blob/b72dfd4443022cab7a0d53e855b55144e61017e9/src/components/ReportActionItem/MoneyRequestPreview.js#L279

What alternative solutions did you explore? (Optional)

We can update the amount text font size in MoneyRequestPreivew to the constant fontSize which is the same with ReportPreview by removing this responsive fontSize and use styles.textHeadline

https://github.com/Expensify/App/blob/b72dfd4443022cab7a0d53e855b55144e61017e9/src/components/ReportActionItem/MoneyRequestPreview.js#L279

We will only need to update the fontSize adjustable for SplitBill which will be based on the length of the amount and the participant of the split bill and the screen is small screen with. We will update this on getAmountFontSizeAndLineHeight function and the font size can be done in the PR.

yh-0218 commented 1 year ago

Proposal

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

Inconsistency in the request money preview text size

What is the root cause of that problem?

We have different style between two. https://github.com/Expensify/App/blob/f8fc21d373be18e645f4d93099256721914c16f5/src/components/ReportActionItem/ReportPreview.js#L221 https://github.com/Expensify/App/blob/f8fc21d373be18e645f4d93099256721914c16f5/src/components/ReportActionItem/MoneyRequestPreview.js#L277-L279

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

We need update ReportPreview like MoneyRequestPreview https://github.com/Expensify/App/blob/f8fc21d373be18e645f4d93099256721914c16f5/src/components/ReportActionItem/ReportPreview.js#L221

<Text
         style={[
                  styles.moneyRequestPreviewAmount,
                  StyleUtils.getAmountFontSizeAndLineHeight(variables.fontSizeXLarge, variables.lineHeightXXLarge, isSmallScreenWidth, windowWidth)
         ]}
>
      {getDisplayAmount()}
</Text>

What alternative solutions did you explore? (Optional)

fedirjh commented 1 year ago

The request money preview text size should be the same in both the chat and the review sections

@lschurr I am not sure about the expected result. I think there is a difference between the two previews :

The report preview inside the parent report is an IOU/Expense preview.
The preview inside the IOU/Expense report is a money request preview.

So I think having a different style for each preview makes sense. cc @Expensify/design Should we unify the styles for both previews?

shawnborton commented 1 year ago

I agree, these should be consistent. @JmillsExpensify @trjExpensify is this a regression from somewhere?

fedirjh commented 1 year ago

@shawnborton What should be expected? Here is the difference between the two styles:

The style of the money request depends on the screen width.
The IOU preview looks to have a fixed style.
IOU Preview style

Money request preview

https://github.com/Expensify/App/assets/36869046/8880cb9f-c68e-4376-8362-fd6452eaca8f

shawnborton commented 1 year ago

I think the preview card should have a consistent max-width (so they all look the same width on desktop), but it's okay if it goes 100% width on smaller screens

fedirjh commented 1 year ago

@shawnborton All cards have the same width, but the issue lies in the text size on the cards.

https://github.com/Expensify/App/assets/36869046/c1b9d100-2b9a-4e57-acaa-46b593860a2c

shawnborton commented 1 year ago

Interesting, I don't know how we ended up with a responsive text size like that but I think we should revert that to stay static. I guess the idea might have been to make the text dynamically smaller in case there were a lot of avatars? @youssef-lr any ideas here?

youssef-lr commented 1 year ago

@shawnborton looks like this change came from this PR to fix the large amounts being cut off in the split preview..

melvin-bot[bot] commented 1 year ago

@lschurr, @fedirjh Whoops! This issue is 2 days overdue. Let's get this updated quick!

fedirjh commented 1 year ago

I guess the idea might have been to make the text dynamically smaller in case there were a lot of avatars

@shawnborton Coming from https://github.com/Expensify/App/issues/24321#issuecomment-1680898494 (Related Slack), The expected is :

When entering 8+ numerals (ex: 12,345,678, 123,456,789, etc), we should expect the text to dynamically decrease in size to allow the characters to fit.

Should we create a different style for the split preview, adjusting the text size based on responsiveness, while keeping the text size static for the other money previews?

trjExpensify commented 1 year ago

I think they should all be consistent, personally.

dubielzyk-expensify commented 1 year ago

Covering for Shawn while he's OOO: Agree that they should all be consistent. If the numbers can't fit, we should shrink to make them fit (but also this is quite rare), but otherwise keep the same static size.

fedirjh commented 1 year ago

@wlegolas @dukenv0307 @yh-0218 Could you please update your proposals to follow the expected behavior in https://github.com/Expensify/App/issues/30275#issuecomment-1786269883 ?

dukenv0307 commented 1 year ago

@fedirjh So we will make both text has the static font-size right?

dukenv0307 commented 1 year ago

My alternative solution have already covered this case https://github.com/Expensify/App/issues/30275#issuecomment-1777644807

fedirjh commented 1 year ago

So we will make both text has the static font-size right?

Not fully static, it should be responsive in case the amount numbers can't fit.

wlegolas commented 1 year ago

Hi @fedirjh, after the discussion, my solution is the same one that @dukenv0307 proposed, as @dukenv0307 already has a proposal, I think there is no reason to update mine.

dukenv0307 commented 1 year ago

Not fully static, it should be responsive in case the amount numbers can't fit.

@fedirjh Because the responsive is used to not cut off the amount if the action is split, I think we should responsive for all cases based on the screen width instead of the amount number can't fit or not.

Let's image the case in a small screen width, one request is split bill with a large amount and split for many people, one request is request money with a small amount. The font size will be not consistent one is large, one is small on the same device.

fedirjh commented 1 year ago

The font size will be not consistent one is large, one is small on the same device.

Yes, that's the current behavior, even with small amount, the text size is not consistent. check this example in IOS:

@dukenv0307 Are you suggesting that we make the IOU preview text size adjustable like the Split bill preview? And in that case, the text will be smaller in all mobile devices? I don't think we want that style; the text looks tiny on a phone.

melvin-bot[bot] commented 1 year ago

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

yh-0218 commented 1 year ago

hi, @fedirjh I just update proposal

fedirjh commented 1 year ago

@dukenv0307 friendly bump for https://github.com/Expensify/App/issues/30275#issuecomment-1787155265

dukenv0307 commented 1 year ago

Are you suggesting that we make the IOU preview text size adjustable like the Split bill preview? And in that case, the text will be smaller in all mobile devices? I don't think we want that style; the text looks tiny on a phone.

@fedirjh Yes, I think that's the only way we can consist the text size for iou preview and report preview. Because responsive font size base on it can fix or not makes iou action itself to be inconsistent.

fedirjh commented 1 year ago

Yes, I think that's the only way we can consist the text size for iou preview and report preview. Because responsive font size base on it can fix or not makes iou action itself to be inconsistent.

I think this challenging, but we can make it as follows:

Set a static font size for all money previews.
For split preview, we can implement the following changes:
- The size of the split preview should be determined based on the length of the text.
- By default, it should have the same size as the other money previews.
- If the amount length is too big, then we can make the split preview size adjustable.

dukenv0307 commented 1 year ago

We will only need to update the fontSize adjustable for SplitBill which will be based on the length of the amount and the participant of the split bill and the screen is small screen with. We will update this on getAmountFontSizeAndLineHeight function and the font size can be done in the PR.

@fedirjh updated alternative solution

fedirjh commented 1 year ago

Sounds good. Let's proceed with @dukenv0307's proposal and use the alternative solution.

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 1 year ago

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

youssef-lr commented 1 year ago

Would love to have @shawnborton and @trjExpensify's input on the proposed solution

fedirjh commented 1 year ago

@youssef-lr Hmm that's what we agreed on https://github.com/Expensify/App/issues/30275#issuecomment-1786269883 :

Agree that they should all be consistent. If the numbers can't fit, we should shrink to make them fit (but also this is quite rare), but otherwise keep the same static size.

dubielzyk-expensify commented 1 year ago

Shawn in OOO for a bit longer, but I'd say that proposed solution is great. That way most people get the same size and experience, but if the number is too large, we adjust. Good stuff!

melvin-bot[bot] commented 1 year ago

@nkuoch, @lschurr, @fedirjh Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 1 year ago

📣 @fedirjh 🎉 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[bot] commented 1 year ago

📣 @dukenv0307 🎉 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[bot] commented 1 year ago

📣 @ayazhussain79 We're missing your Upwork ID to automatically send you an offer for the Reporter role. Once you apply to the Upwork job, your Upwork ID will be stored and you will be automatically hired for future jobs!

lschurr commented 1 year ago

Alrighty, I've assigned @dukenv0307

dukenv0307 commented 1 year ago

@fedirjh The PR is ready for review.

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

https://github.com/Expensify/App/pull/30960

If no regressions arise, payment will be issued on 2023-11-22. :confetti_ball:

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

[ ] External issue reporter
[ ] Contributor that fixed the issue
[ ] Contributor+ that helped on the issue and/or PR

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

@fedirjh requires payment offer (Reviewer)
@dukenv0307 requires payment offer (Contributor)
@ayazhussain79 requires payment

melvin-bot[bot] commented 1 year ago

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:

[ ] [@fedirjh] The PR that introduced the bug has been identified. Link to the PR:
[ ] [@fedirjh] 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:
[ ] [@fedirjh] 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:
[ ] [@fedirjh] Determine if we should create a regression test for this bug.
[ ] [@fedirjh] 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.
[ ] [@lschurr] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

lschurr commented 1 year ago

@fedirjh could you work through the checklist for this one?

fedirjh commented 12 months ago

BugZero Checklist:

[x] Link to the PR: https://github.com/Expensify/App/pull/26280
[x] Link to comment: https://github.com/Expensify/App/pull/26280#discussion_r1399301388
[x] Link to discussion: N/A Nothing to add to the checklist.
[x] Determine if we should create a regression test for this bug: Let's have a regression text for the amount text size

Regression Test Proposal

Open the app and navigate to the report with a user.
Create a money request with same user.
Create a split request with same user.
Verify that you see both the money request preview and the split request preview.
Verify that the font size of the amount text is consistent between the money request preview and the split bill preview.
- Do we agree 👍 or 👎

lschurr commented 12 months ago

Payment summary:

Bug reporter: @ayazhussain79 $50
Contributor: @dukenv0307 $500
Contributor+: @fedirjh $500

lschurr commented 12 months ago

@ayazalavi could you apply to the job in Upwork? https://www.upwork.com/jobs/~01a260f6acb4caa392

ayazhussain79 commented 12 months ago

@lschurr You've mentioned the wrong person. I've applied on Upwork. Thank you.

Expensify / App