[$500] IOU - Padding below total increases for unread messages on mobile devices #30360

kbecciv · 2023-10-25T16:13:11Z

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.2
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: @dhanashree-sawant
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1698067716752699

Action Performed:

Open the app and login with user A
Open the app with any other device and login with user B
From user B, send request money to user A
Open user B request money in user A and mark message as unread
Observe the padding between 'total' and 'New' line
Revisit the request money untill 'New' line disappears and now observe the padding between 'total' and line below total

Expected Result:

App should have equal padding below total irrespective of presence of 'New' line

Actual Result:

On mobile devices, padding between total and 'New' line is way more then padding when there is no 'New' line

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Android: Native

android.native.padding.below.total.mp4

Android: mWeb Chrome

android.chrome.padding.below.total.mp4

iOS: Native

ios.native.padding.below.total.mov

iOS: mWeb Safari

ios.safari.padding.below.total.mov

MacOS: Chrome / Safari

MacOS: Desktop

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~0158fbfd6d89166dba
Upwork Job ID: 1717212733665116160
Last Price Increase: 2023-10-25
Automatic offers:

situchan | Contributor | 27456744
dhanashree-sawant | Reporter | 27456747
DylanDylann | Contributor | 0

melvin-bot · 2023-10-25T16:13:16Z

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

melvin-bot · 2023-10-25T16:13:16Z

Job added to Upwork: https://www.upwork.com/jobs/~0158fbfd6d89166dba

melvin-bot · 2023-10-25T16:13:18Z

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 · 2023-10-25T16:13:22Z

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

Nathan-Mulugeta · 2023-10-25T16:53:08Z

Proposal

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

Padding below total increases for unread messages on mobile devices

What is the root cause of that problem?

The root cause of this problem is found in here:

App/src/CONST.ts

Lines 126 to 131 in cc3c51c

    
           MONEY_REPORT: { 
        
               SMALL_SCREEN: { 
        
                   IMAGE_HEIGHT: 300, 
        
                   CONTAINER_MINHEIGHT: 280, 
        
                   VIEW_HEIGHT: 220, 
        
               },

The size of the CONTAINER_MINHEIGHT for SMALL_SCREEN is the same as the size of CONTAINER_MINHEIGHT for WIDE_SCREEN.

Furthermore, a discrepancy on the web arises when props.shouldShowHorizontalRule is set to false. The SpacerView component persists within the DOM, causing an undue gap between the unreadIndicator and the total amount.

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

We need to decrease the size of the CONTAINER_MINHEIGHT from 280 to 260 for SMALL_SCREEN sizes. Which fixes the issue on small screens as we are adjusting the min height accordingly.
The changes should look like this:

  SMALL_SCREEN: {
                IMAGE_HEIGHT: 300,
                CONTAINER_MINHEIGHT: 260, // Update this line 
                VIEW_HEIGHT: 220,
            },

Amend the code snippet at MoneyReportView.js to conditionally render the SpacerView component as follows:

{props.shouldShowHorizontalRule && (
                    <SpacerView
                        shouldShow={props.shouldShowHorizontalRule}
                        style={[props.shouldShowHorizontalRule ? styles.reportHorizontalRule : {}]}
                    />
                )}

Implementing these changes will address the excessive padding issue on both mobile devices and web interfaces by adjusting CONTAINER_MINHEIGHT appropriately and rendering the SpacerView component conditionally when required.

What alternative solutions did you explore? (Optional)

NA

Results

Native

native.mp4

Web

[web.webm](https://github.com/Expensify/App/assets/111440031/2f40b5f5-8492-47e6-9270-9321b0a4d33b)

tienifr · 2023-10-26T09:44:44Z

Proposal

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

IOU - Padding below total increases for unread messages on mobile devices

What is the root cause of that problem?

We have 2 problems here:

In MoneyRequestView, we're using getReportWelcomeContainerStyle to get the style

App/src/components/ReportActionItem/MoneyRequestView.js

Line 163 in cd851f5

And we fix the minHeight, in this case, it's 280px

App/src/styles/StyleUtils.ts

Line 944 in cd851f5

minHeight: emptyStateBackground.SMALL_SCREEN.CONTAINER_MINHEIGHT,

but, if we show the unread marker, the SpacerView will be hidden, so the minHeight at that time should be 260px

We already have the logic to add marginBottom: 8px for the preview. But it's not correct, because we set the top of unread marker is -10px

App/src/pages/home/report/ReportActionItem.js

Line 610 in 414d591

    
           return <View style={[props.shouldHideThreadDividerLine && !isNormalCreatedAction && styles.mb2]}>{content}</View>;

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

Solution 1:

If shouldShowHorizontalRule is false, we should minus the minHeight by 20px

            minHeight: emptyStateBackground.SMALL_SCREEN.CONTAINER_MINHEIGHT - (shouldShowHorizontalRule ? 0: 20),

Change styles.mb2 in this line to {marginBottom: 10}

Solution 2:

We can show the SpacerView no matter the shouldShowHorizontalRule is to preserve the space, and remove borderColor (divider) if shouldShowHorizontalRule is false
Then remove the marginBottom logic here
Update top: -20 of UnreadActionIndicator when shouldShowHorizontalRule is true, otherwise top:-10

I prefer the solution 2 because, it not only preserves the space between new line and total, but also the distance between new line and money preview

Result

Screen.Recording.2023-10-26.at.17.15.28.mov

ArekChr · 2023-10-26T10:55:21Z

@tienifr, I’ve gone through your proposal, and it seems good overall. Regarding the second solution, my only concern is negative spacing. Generally, avoiding such styles is a good idea as they can lead to inconsistency in the future. What are your thoughts on fixing this issue by avoiding negative spacing?

Nathan-Mulugeta · 2023-10-26T10:58:06Z

What are your thoughts on my proposal? @ArekChr

tienifr · 2023-10-27T02:43:28Z

@ArekChr you mean the top: -20 of UnreadActionIndicator? I think we have 2 cases that apply UnreadActionIndicator. The first one is below the created action, in this case, we should hide the horizontal rule -> top: -20
The second one is below the chat action -> top: -10 as what we already did here

alexpensify · 2023-10-30T23:53:13Z

@ArekChr - can you review the responses when you get a chance? Thanks!

ArekChr · 2023-10-31T12:03:01Z

@garrettmknight Hello, I am wrapping up some urgent matters today, and I will be OOO from tomorrow to Monday. Can we assign another C+ here so as not to block the task?

situchan · 2023-10-31T12:15:17Z

I can take over

melvin-bot · 2023-10-31T14:56:06Z

📣 @situchan 🎉 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 · 2023-10-31T14:56:11Z

📣 @dhanashree-sawant 🎉 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

alexpensify · 2023-10-31T14:57:16Z

Thank you @ArekChr for this update!

@situchan you are assigned now. Let's keep this one moving forward.

situchan · 2023-10-31T16:21:28Z

@tienifr does your solution also fix web as well?

Screen.Recording.2023-10-31.at.10.19.14.PM.mov

tienifr · 2023-11-01T02:59:33Z

@situchan Yes, did you test my second solution? I prefer the second one, if we apply the first solution, we need to disable animation when the minHeight change. Unfortunately, I didn't find any solutions for that edge case.

I tested the second solution and it worked well (the attached video here)

Nathan-Mulugeta · 2023-11-01T03:04:43Z

Hey @situchan ,have you got the chance to check my proposal. I would love to get a feedback on it please.

alexpensify · 2024-03-26T00:34:29Z

Thanks! Regressions are always a priority.

alexpensify · 2024-03-27T20:04:24Z

No update here

alexpensify · 2024-03-29T17:37:40Z

The PR is in a draft-- making progress

dragnoir · 2024-03-29T23:52:28Z

PR ready #39297

alexpensify · 2024-04-05T22:37:43Z

Update: The PR is waiting for @Beamanator to review it.

alexpensify · 2024-04-12T23:31:46Z

Update: The PR is moving forward

alexpensify · 2024-04-16T23:10:49Z

Weekly Update: The PR is going through some polish updates to help the design.

melvin-bot · 2024-04-23T07:35:43Z

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

Beamanator · 2024-04-23T07:37:19Z

FYI I'm assigning @DylanDylann as C+ to review - but I haven't unassigned @situchan since they also did a review a bit ago.

alexpensify · 2024-04-30T22:35:20Z

Weekly Update: Looks like the PR is going through a final review.

melvin-bot · 2024-05-02T15:10:49Z

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

alexpensify · 2024-05-07T21:58:46Z

@DylanDylann - can we get an update if this Deploy Blocker is accurate or an error? Thanks!

DylanDylann · 2024-05-08T02:40:16Z

@alexpensify Yeah, It is a regression

dragnoir · 2024-05-08T09:25:38Z

@alexpensify When I submitted the PR for review, the new feature "invoice" was not there. The issue happened with the invoice report.

alexpensify · 2024-05-14T22:17:42Z

Thank you for this context! It looks like the automation failed here and payment is due for this one. I'll work on that process.

alexpensify · 2024-05-14T22:44:31Z

Please disregard the last comment.

Ok, flagging this notice: #30360 (comment)

@dragnoir please apply here: https://www.upwork.com/jobs/~0104d969f5990f36bd

Everyone else will be paid via this link: https://www.upwork.com/jobs/~0158fbfd6d89166dba

After, I can carry on with the payment process. Thanks!

dragnoir · 2024-05-15T08:03:12Z

@alexpensify applied, thank you

alexpensify · 2024-05-15T22:45:13Z

Payouts due: 2024-05-13 - delayed due to automation failure

Reported by: $50 @dhanashree-sawant
Contributor: $500 @dragnoir
Contributor+: $250 for the partial review @situchan
Contributor+: $500 @DylanDylann

Upwork job is here. The PR was submitted before the payment changes in April. Also, this issue was created in 2023, so that's why there is a reporter bonus.

alexpensify · 2024-05-15T22:46:30Z

@dragnoir - I sent you an offer in Upwork. Please accept, and I can complete the payment process.

dragnoir · 2024-05-16T07:33:42Z

@alexpensify offer accepted, thank you

alexpensify · 2024-05-17T15:38:54Z

All set, everyone has been paid via Upwork!

Heads up, I will be offline until Tuesday, May 28, 2024, and will not actively watch over this GitHub during that period.

If anything urgent is needed here, please ask for help in the #expensify-open-source Slack Room-- thanks!

kbecciv added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Oct 25, 2023

melvin-bot bot assigned alexpensify Oct 25, 2023

melvin-bot bot changed the title ~~IOU - Padding below total increases for unread messages on mobile devices~~ [$500] IOU - Padding below total increases for unread messages on mobile devices Oct 25, 2023

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 25, 2023

melvin-bot bot assigned ArekChr Oct 25, 2023

melvin-bot bot added the Overdue label Oct 30, 2023

melvin-bot bot removed the Overdue label Oct 30, 2023

alexpensify assigned situchan and unassigned ArekChr Oct 31, 2023

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

melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Mar 29, 2024

Beamanator assigned DylanDylann Apr 23, 2024

Beamanator mentioned this issue Apr 26, 2024

Payment issue for E/App PR#39297 #41080

Closed

melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels May 2, 2024

CortneyOfstad mentioned this issue May 15, 2024

[$250] Send invoice - App crashes after creating invoice #41504

Closed

alexpensify closed this as completed May 17, 2024

[$500] IOU - Padding below total increases for unread messages on mobile devices #30360

[$500] IOU - Padding below total increases for unread messages on mobile devices #30360

Comments

kbecciv commented Oct 25, 2023 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

melvin-bot bot commented Oct 25, 2023

melvin-bot bot commented Oct 25, 2023

melvin-bot bot commented Oct 25, 2023

Bug0 Triage Checklist (Main S/O)

melvin-bot bot commented Oct 25, 2023

Nathan-Mulugeta commented Oct 25, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Results

tienifr commented Oct 26, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

Result

ArekChr commented Oct 26, 2023

Nathan-Mulugeta commented Oct 26, 2023

tienifr commented Oct 27, 2023 • edited Loading

alexpensify commented Oct 30, 2023

ArekChr commented Oct 31, 2023

situchan commented Oct 31, 2023

melvin-bot bot commented Oct 31, 2023

melvin-bot bot commented Oct 31, 2023

alexpensify commented Oct 31, 2023

situchan commented Oct 31, 2023

tienifr commented Nov 1, 2023 • edited Loading

Nathan-Mulugeta commented Nov 1, 2023

alexpensify commented Mar 26, 2024

alexpensify commented Mar 27, 2024

alexpensify commented Mar 29, 2024

dragnoir commented Mar 29, 2024

alexpensify commented Apr 5, 2024

alexpensify commented Apr 12, 2024

alexpensify commented Apr 16, 2024

melvin-bot bot commented Apr 23, 2024

Beamanator commented Apr 23, 2024

alexpensify commented Apr 30, 2024

melvin-bot bot commented May 2, 2024

alexpensify commented May 7, 2024

DylanDylann commented May 8, 2024

dragnoir commented May 8, 2024

alexpensify commented May 14, 2024

alexpensify commented May 14, 2024

dragnoir commented May 15, 2024

alexpensify commented May 15, 2024 • edited Loading

alexpensify commented May 15, 2024

dragnoir commented May 16, 2024

alexpensify commented May 17, 2024

Heads up, I will be offline until Tuesday, May 28, 2024, and will not actively watch over this GitHub during that period.

kbecciv commented Oct 25, 2023 •

edited by melvin-bot bot

Loading

Nathan-Mulugeta commented Oct 25, 2023 •

edited

Loading

tienifr commented Oct 26, 2023 •

edited

Loading

tienifr commented Oct 27, 2023 •

edited

Loading

tienifr commented Nov 1, 2023 •

edited

Loading

alexpensify commented May 15, 2024 •

edited

Loading