[HOLD for payment 2024-06-06] [$500] Empty state image has too much overlap with message in attachment thread

[m-natarajan]

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.4.38-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: @shawnborton
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1707401686727289

Action Performed:

1. Send a message with a vertical (portrait) image. Then create a thread on this image and view the thread.

Expected Result:

The empty state image/background pattern should not have so much overlap with the message.

Actual Result:

The empty state pattern has a LOT of overlap with the message, almost sitting directly behind it.

Workaround:

unknown

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

Add any screenshot/video evidence

With vertical tall image

with normal image

without image

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0180e51b2d8c0c04ec
• Upwork Job ID: 1755638484200894464
• Last Price Increase: 2024-04-24
• Automatic offers:
• cubuspl42 | Reviewer | 0
• rayane-djouah | Contributor | 0

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0180e51b2d8c0c04ec

[melvin-bot]

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

[melvin-bot]

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

[rayane-djouah]

@shawnborton could you please share mockups of the expected result ?

[neonbhai]

Proposal

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

Empty state image has too much overlap with message in attachment thread

What is the root cause of that problem?

This happens as we have and incorrect style justifyContentEnd here:

App/src/pages/home/report/ReportActionItemParentAction.tsx

Line 74 in e309896

<View style={[StyleUtils.getReportWelcomeContainerStyle(isSmallScreenWidth), styles.justifyContentEnd]}>

This makes the AnimatedEmptyStateBackground image appear behind the content.

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

We should move the styles.justifyContentEnd from this View to the View just after AnimatedEmptyStateBackground:

App/src/pages/home/report/ReportActionItemParentAction.tsx

Line 76 in e309896

<View style={[styles.p5, StyleUtils.getReportWelcomeTopMarginStyle(isSmallScreenWidth)]} />

Result:

Before

Screen.Recording.2024-02-09.at.12.03.48.AM.mov

After

[shawnborton]

@rayane-djouah shared a rough mockup for you here

[rayane-djouah]

Proposal

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

The empty state image is excessively overlapping with the message within the attachment thread

What is the root cause of that problem?

We are applying an incorrect styling attribute, specifically justifyContentEnd, within the following component:

App/src/pages/home/report/ReportActionItemParentAction.tsx

Line 74 in 43e81a1

<View style={[StyleUtils.getReportWelcomeContainerStyle(isSmallScreenWidth), styles.justifyContentEnd]}>

This particular styling causes the AnimatedEmptyStateBackground image to be positioned behind the thread content, leading to the overlap issue.

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

we should remove , styles.justifyContentEnd here:

App/src/pages/home/report/ReportActionItemParentAction.tsx

Line 74 in 43e81a1

<View style={[StyleUtils.getReportWelcomeContainerStyle(isSmallScreenWidth), styles.justifyContentEnd]}>

Result:

But a lot of overlap is still applied because the report message top margin value here is not enough:

App/src/pages/home/report/ReportActionItemParentAction.tsx

Line 76 in 43e81a1

<View style={[styles.p5, StyleUtils.getReportWelcomeTopMarginStyle(isSmallScreenWidth)]} />

after removing the justifyContentEnd style, an overlap of 135px is still applied.
the report first item top margin value is here:

App/src/CONST.ts

Line 153 in 176675b

VIEW_HEIGHT: 275,

we are also applying a padding here that adds 40px:

App/src/pages/home/report/ReportActionItemParentAction.tsx

Line 76 in 43e81a1

<View style={[styles.p5, StyleUtils.getReportWelcomeTopMarginStyle(isSmallScreenWidth)]} />

the background image size is 450px here :

App/src/CONST.ts

Line 151 in 176675b

IMAGE_HEIGHT: 450,

overlap value = 450px - (275px + 40px) = 135px

To move the background image a little to the top and use a 60px overlap in all chat reports views,
we should modify VIEW_HEIGHT here to use 240px for small screens and 390px for wide screens.

        SMALL_SCREEN: {
            IMAGE_HEIGHT: 300,
            CONTAINER_MINHEIGHT: 200,
            VIEW_HEIGHT: 240,
        },
        WIDE_SCREEN: {
            IMAGE_HEIGHT: 450,
            CONTAINER_MINHEIGHT: 500,
            VIEW_HEIGHT: 390,
        },
        MONEY_OR_TASK_REPORT: {
            SMALL_SCREEN: {
                IMAGE_HEIGHT: 300,
                CONTAINER_MINHEIGHT: 280,
                VIEW_HEIGHT: 240,
            },
            WIDE_SCREEN: {
                IMAGE_HEIGHT: 450,
                CONTAINER_MINHEIGHT: 280,
                VIEW_HEIGHT: 390,
            },
        },

The formula for choosing VIEW_HEIGHT is (after removing the 40px padding in thread screen):
for WIDE_SCREEN:
overlap between the report message and the background image= 450px background image height - VIEW_HEIGHT value
for SMALL_SCREEN:
overlap between the report message and the background image= 300px background image height - VIEW_HEIGHT value

the above values (240 and 390) gives 60px overlap.

we should get the exact overlap value from the design team.

I asked the design team here for the overlap value, and they fixed a value of 60px for both wide and small screens.

and here:

App/src/pages/home/report/ReportActionItemParentAction.tsx

Line 76 in 43e81a1

<View style={[styles.p5, StyleUtils.getReportWelcomeTopMarginStyle(isSmallScreenWidth)]} />

remove styles.p5 that we don't need (it adds 40px padding that get added as a top margin to the message).

<View style={[StyleUtils.getReportWelcomeTopMarginStyle(isSmallScreenWidth)]} />

Result with VIEW_HEIGHT: 390 for wide screens:

Result for wide screens:

Dimensions details

AnimatedEmptyStateBackground image size:

report message top margin view size:

Result for small screens:

Dimensions details

AnimatedEmptyStateBackground image size:

report message top margin view size:

What alternative solutions did you explore? (Optional)

If we want to use a 60px overlap value in the thread screen only and keep 135px in other screens, we can do the following:

we should add here this code to make the change only for thread reports:

        THREAD_REPORT: {
          SMALL_SCREEN: {
              IMAGE_HEIGHT: 300,
              CONTAINER_MINHEIGHT: 200,
              VIEW_HEIGHT: 240,  //ajust the view height value here
          },
          WIDE_SCREEN: {
              IMAGE_HEIGHT: 450,
              CONTAINER_MINHEIGHT: 500,
              VIEW_HEIGHT: 390, //ajust the view height value here
          },
        },

and here:

App/src/styles/utils/index.ts

Lines 724 to 725 in 43e81a1

	function getReportWelcomeTopMarginStyle(isSmallScreenWidth: boolean, isMoneyOrTaskReport = false): ViewStyle {
	const emptyStateBackground = isMoneyOrTaskReport ? CONST.EMPTY_STATE_BACKGROUND.MONEY_OR_TASK_REPORT : CONST.EMPTY_STATE_BACKGROUND;

add isThreadReport :

function getReportWelcomeTopMarginStyle(isSmallScreenWidth: boolean, isMoneyOrTaskReport = false, isThreadReport = false): ViewStyle {
    let emptyStateBackground;
    if (isMoneyOrTaskReport) {
        emptyStateBackground = CONST.EMPTY_STATE_BACKGROUND.MONEY_OR_TASK_REPORT;
    }
    else if (isThreadReport) {
        emptyStateBackground = CONST.EMPTY_STATE_BACKGROUND.THREAD_REPORT;
    }
    else {
        emptyStateBackground = CONST.EMPTY_STATE_BACKGROUND;
    }
// rest remains the same

and here:

App/src/pages/home/report/ReportActionItemParentAction.tsx

Line 76 in 43e81a1

<View style={[styles.p5, StyleUtils.getReportWelcomeTopMarginStyle(isSmallScreenWidth)]} />

pass true for isThreadReport and remove styles.p5 that we don't need (it adds 40px padding that get added as a top margin to the message).

<View style={[StyleUtils.getReportWelcomeTopMarginStyle(isSmallScreenWidth, false, true)]} />

[cubuspl42]

@shawnborton Does this require any extra approval, as it's a user-observable change? Or is your approval enough?

[rayane-djouah]

Proposal

Updated

[shawnborton]

@cubuspl42 I think just having approval from the @Expensify/design is enough here. @rayane-djouah was very helpful in Slack in showing us what different values might look like.

One thing we might consider is how this affects ALL empty states, as in, maybe we should make this change across all room types (DM, #room, report, expense, etc) so that we get the consistent 60px overlap no matter where you are.

[rayane-djouah]

One thing we might consider is how this affects ALL empty states, as in, maybe we should make this change across all room types (DM, #room, report, expense, etc) so that we get the consistent 60px overlap no matter where you are.

The changes will apply to all thread reports, no matter from what type of chat they are created from (DM, #room, report, expense, etc), ensuring a consistent 60px overlap across all empty states.

[shawnborton]

Got it, but I'm thinking not just thread reports but just regular DMs, #rooms, etc. Basically this part:

[rayane-djouah]

Here is the current situation of the app:

Currently we are setting a custom top margin ( VIEW_HEIGHT ) for money report screen here:

App/src/CONST.ts

Lines 156 to 165 in 176675b

	SMALL_SCREEN: {
	IMAGE_HEIGHT: 300,
	CONTAINER_MINHEIGHT: 280,
	VIEW_HEIGHT: 220,
	},
	WIDE_SCREEN: {
	IMAGE_HEIGHT: 450,
	CONTAINER_MINHEIGHT: 280,
	VIEW_HEIGHT: 275,
	},

which gives a 175px overlap on wide screens and 80px overlap on small screens:

Money Report Screen

For everything else we are setting the top margin ( VIEW_HEIGHT ) here:

App/src/CONST.ts

Lines 145 to 154 in 176675b

	SMALL_SCREEN: {
	IMAGE_HEIGHT: 300,
	CONTAINER_MINHEIGHT: 200,
	VIEW_HEIGHT: 185,
	},
	WIDE_SCREEN: {
	IMAGE_HEIGHT: 450,
	CONTAINER_MINHEIGHT: 500,
	VIEW_HEIGHT: 275,
	},

which gives a 175px overlap on wide screens and 115px overlap on small screens:

Other screens

For thread screen, we currently place the background image behind the report first message, resulting in a 100% overlap.

Thread screen

My proposal suggest modifying the logic for thread screen only by implementing a custom VIEW_HEIGHT value for thread screen to apply a 60px overlap on small and wide screens while maintaining the existing logic for other screen types.

@shawnborton, if you think we should also revise the overlap values for other screen types, please let us know. Thank you!

[rayane-djouah]

should we apply 60px overlap to thread screen and this "report with welcome message" screen and keep other screens overlap values unchanged? or is there any other adjustments to other screens also?

[shawnborton]

I'm mostly just thinking that we should pick a specific value, in this case 60px, for the overlap, and apply that consistently to every single chat view. No matter if it's a room, a thread, a report, a workspace chat, a task, etc. Let me know if that makes sense.

Also cc @grgia to take a look at Rayane's comment above since I think you implemented some of this?

[rayane-djouah]

The empty state design was implemented in this PR and the custom logic for money report screen was added in this PR.

If we want to apply 60px overlap to every single chat view, we can modify VIEW_HEIGHT to (240px for small and 390px for wide) values here instead of adding new values and custom logic for thread screen.

[rayane-djouah]

Proposal

Updated to use a 60px overlap value in all chat report views instead of applying it only to the thread report.

[rayane-djouah]

Result when applying 60px overlap in all chat report view:

Result

Are they looking good?

[dragnoir]

Proposal

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

Image on background not in correct position

What is the root cause of that problem?

We have a style applied here:

App/src/pages/home/report/ReportActionItemParentAction.tsx

Line 74 in 287613e

<View style={[StyleUtils.getReportWelcomeContainerStyle(isSmallScreenWidth), styles.justifyContentEnd]}>

styles.justifyContentEnd is pushing the background image to the bottom. This only happen to the thread pages.

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

Just removing the styles.justifyContentEnd will fix the issue.

-  <View style={[StyleUtils.getReportWelcomeContainerStyle(isSmallScreenWidth), styles.justifyContentEnd]}>
+  <View style={[StyleUtils.getReportWelcomeContainerStyle(isSmallScreenWidth)>

POC:

On the other pages, the parent of <AnimatedEmptyStateBackground /> does not include styles.justifyContentEnd like:

• MoneyReportView:
  

  App/src/components/ReportActionItem/MoneyReportView.tsx

  Lines 67 to 68 in 287613e

  	<View style={[StyleUtils.getReportWelcomeContainerStyle(isSmallScreenWidth, true)]}>
  	<AnimatedEmptyStateBackground />

• ReportActionItem
  

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

  Lines 630 to 631 in 287613e

  	<View style={[StyleUtils.getReportWelcomeContainerStyle(props.isSmallScreenWidth, true)]}>
  	<AnimatedEmptyStateBackground />

[shawnborton]

@rayane-djouah that feels good to me. @cubuspl42 @johncschuster I would love to have @rayane-djouah work on this as they've been super helpful in exploring solutions for this one.

[melvin-bot]

@cubuspl42, @johncschuster Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[johncschuster]

@shawnborton / @yuwenmemon I'm having a hard time understanding where your conversation has landed. @yuwenmemon, are you confirming there was a regression?

[yuwenmemon]

Yes, correct. There's a regression.

[johncschuster]

Thanks! I've updated the payment summary above to indicate that.

[eh2077]

@shawnborton Yeah, we should get regression fixed before issuing payment.
@rayane-djouah Can you take a look at this comment #42795 (comment) and fix the regression from this PR?

I don't think that #42795 is caused by my PR. I think the root cause of that bug is we are displaying a single expense twice. The empty state image is designed to be displayed in the first action of a report. if we display two report actions with the image, one of them will be covered by the image. I think that the duplicate expense action bug is what need to fix in #42795. Do you agree?

As @rayane-djouah seems to be OOO, let me try to continue with the discussion. I also think the UI issue of #42795 shouldn’t be a regression from this PR if following is true

From design perspective, we don't want to show two or more money requests in the MoneyRequestView (no matter if the request is editable or not), right?

This is the bug screenshot - the expense is displayed twice

The normal ones look like
This is editable

This is not editable

@shawnborton Can you help to drive clarity here? It'll also be helpful for the other issue #42795

cc @yuwenmemon @stitesExpensify

[shawnborton]

Correct, we definitely do not want to display the expense information twice like that.

[eh2077]

@shawnborton Even if they are different expenses, we also don't want to display more than one expense in this page, right?

[shawnborton]

If we have multiple expenses in the same report, we use a different view where we show expense preview cards, not the full expense details view:

[eh2077]

I see. Then clicking a preview card will only show details of an expense. If so, I think the implementation of this PR should be fine and it shouldn't be blamed for the unreasonable use case (bug) - show details of an expense twice in one page.

[johncschuster]

This is still actively being discussed. This is just a bump for Melvin. Carry on!

[rayane-djouah]

I have returned from being out of office, sorry for the delays here.

@shawnborton - What do you think about @eh2077's comment ^ above?

@rayane-djouah - do you agree that #42760 was caused by this though?

Yes, it was fixed by #42629

[shawnborton]

Sorry - what is the exact question you need me to answer?

[johncschuster]

@rayane-djouah, bump on the above

[rayane-djouah]

Sorry - what is the exact question you need me to answer?

I think this one: #36166 (comment)

I think the only regression the PR caused is this one: #42760, which is now resolved. Therefore, we can complete the payment based on the payment summary and close the issue.

[johncschuster]

@shawnborton ^^

[shawnborton]

Whatever you all think is best works for me. Just noting that a bunch of regressions came out of this one, and it seems like we have a new one that needs to be addressed here too: #43247

[johncschuster]

Discussing payment. Will update the issue shortly.

[johncschuster]

I spoke with @yuwenmemon about this, and it sounds like there were two regressions within the 7-day waiting period: #42760 and #34640. Have I understood that correctly? Once I have a sense of the regressions caused, I'll update the payment summary so we can button this up.

Thanks for your patience everyone!

[rayane-djouah]

Sorry - did you link the wrong issue (#34640)?

I'll try to drive clarity here.

The bugs that blame this PR are:

• Android - IOU - Info section under total is cut off #42760 This was a regression from this PR and was fixed in fix first action item background overlap on the next item #42629.
• [$250] Background in Search page RHP is duplicated and has unnecessary empty space #43247 This is not reproducible, and we can close it.
• [HOLD for payment 2024-07-24] [$250] Submitting Expense from Self DM Results in Duplicate Submissions #42795 This should not blame my PR as the root cause is that we are displaying two expenses. If we reverse my PR locally, the duplicate expenses bug will still occur, and the difference is that there will be a space between them. There is already a proposal that explains the root cause, which is not related to my PR.

Note that because we removed the hard-coded space for the background image, Any report action or expense that may be incorrectly displayed above the first report action that has the background image will be overlapped by the image because of zIndex values in this code:

App/src/components/InvertedFlatList/CellRendererComponent.tsx

Lines 10 to 28 in 5f45b4c

	function CellRendererComponent(props: CellRendererComponentProps) {
	return (
	<View
	// eslint-disable-next-line react/jsx-props-no-spreading
	{...props}
	style={[
	props.style,
	/**
	* To achieve absolute positioning and handle overflows for list items,
	* it is necessary to assign zIndex values. In the case of inverted lists,
	* the lower list items will have higher zIndex values compared to the upper
	* list items. Consequently, lower list items can overflow the upper list items.
	* See: https://github.com/Expensify/App/issues/20451
	*/
	{zIndex: -props.index, position: 'relative'},
	]}
	/>
	);
	}

However, this is correct behaviour because no report action should be displayed above the first report action with the background image. If a bug like this occurs, it is because there is a duplicate report action with the background image or the order of report actions is incorrect; in this case, the root cause of the bug should be fixed. If we reverse my PR, the bug will still occur.

I think the only bug that my PR may be blamed for is #42760

It would be nice if we consider that #35921 was combined with this issue, and it took too much effort with two PRs worked on: #36765 #38449

But I'm okay with whatever you find fair.

[johncschuster]

@rayane-djouah Ah, I did link the wrong one. Sorry about that!

Thank you for writing that out so clearly! I've spoken to the team, and we've agreed on the payment amounts originally dictated above. I'll copy them here for ease:

Payment Summary:

Contributor: @rayane-djouah - $250 - paid via Upwork - PAID
Contributor+: @eh2077 - $250 - paid via Upwork - PAID