[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
• [x] 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[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[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: https://github.com/Expensify/App/blob/e30989663188effdd4806d2b5f7dac0ded93f648/src/pages/home/report/ReportActionItemParentAction.tsx#L74

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: https://github.com/Expensify/App/blob/e30989663188effdd4806d2b5f7dac0ded93f648/src/pages/home/report/ReportActionItemParentAction.tsx#L76

Result:

Before

https://github.com/Expensify/App/assets/64629613/5a2136f9-756a-439b-b5eb-4d021bc53398

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: https://github.com/Expensify/App/blob/43e81a1a26411bce2838557c9fbfccaa5fb23ff3/src/pages/home/report/ReportActionItemParentAction.tsx#L74

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:

https://github.com/Expensify/App/blob/43e81a1a26411bce2838557c9fbfccaa5fb23ff3/src/pages/home/report/ReportActionItemParentAction.tsx#L74

Result:

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

https://github.com/Expensify/App/blob/43e81a1a26411bce2838557c9fbfccaa5fb23ff3/src/pages/home/report/ReportActionItemParentAction.tsx#L76-L76

after removing the justifyContentEnd style, an overlap of 135px is still applied. the report first item top margin value is here: https://github.com/Expensify/App/blob/176675b03128c5cabd367df6053a118c7869c114/src/CONST.ts#L153-L153 we are also applying a padding here that adds 40px: https://github.com/Expensify/App/blob/43e81a1a26411bce2838557c9fbfccaa5fb23ff3/src/pages/home/report/ReportActionItemParentAction.tsx#L76-L76 the background image size is 450px here : https://github.com/Expensify/App/blob/176675b03128c5cabd367df6053a118c7869c114/src/CONST.ts#L151-L151 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: https://github.com/Expensify/App/blob/43e81a1a26411bce2838557c9fbfccaa5fb23ff3/src/pages/home/report/ReportActionItemParentAction.tsx#L76-L76 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:

https://github.com/Expensify/App/blob/43e81a1a26411bce2838557c9fbfccaa5fb23ff3/src/styles/utils/index.ts#L724-L725

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: https://github.com/Expensify/App/blob/43e81a1a26411bce2838557c9fbfccaa5fb23ff3/src/pages/home/report/ReportActionItemParentAction.tsx#L76-L76 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:

https://github.com/Expensify/App/blob/176675b03128c5cabd367df6053a118c7869c114/src/CONST.ts#L156-L165

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:

https://github.com/Expensify/App/blob/176675b03128c5cabd367df6053a118c7869c114/src/CONST.ts#L145-L154

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: https://github.com/Expensify/App/blob/287613ea925964f66753892daaa3a779d61d0e2d/src/pages/home/report/ReportActionItemParentAction.tsx#L74

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: https://github.com/Expensify/App/blob/287613ea925964f66753892daaa3a779d61d0e2d/src/components/ReportActionItem/MoneyReportView.tsx#L67-L68

• ReportActionItem https://github.com/Expensify/App/blob/287613ea925964f66753892daaa3a779d61d0e2d/src/pages/home/report/ReportActionItem.js#L630-L631

[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[bot]]

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

[cubuspl42]

@shawnborton

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.

Would you confirm the scope here?

[cubuspl42]

C+ reviewed 🎀 👀 🎀

I agree with @shawnborton that we should assign @rayane-djouah

[melvin-bot[bot]]

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

[shawnborton]

Would you confirm the scope here?

Haha, yes, let's fix this for ALL room/chat types.

[melvin-bot[bot]]

📣 @cubuspl42 🎉 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]]

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

[johncschuster]

Sounds great! I've just assigned @rayane-djouah 🎉

[miljakljajic]

Another similar example: https://github.com/Expensify/App/issues/35921

[cubuspl42]

I understand that https://github.com/Expensify/App/issues/35921 was closed in favor of this issue. Am I right?

Our PR (https://github.com/Expensify/App/pull/36765) has already been merged.

@rayane-djouah Could you check if the solution we implemented handled what is described in https://github.com/Expensify/App/issues/35921?

[miljakljajic]

If not, we can reopen. I just closed in reference to Shawn's comment on our issue.

[rayane-djouah]

@rayane-djouah Could you check if the solution we implemented handled what is described in https://github.com/Expensify/App/issues/35921?

No, the solution to https://github.com/Expensify/App/issues/35921 is different, it was described here. @miljakljajic please reopen.

[cubuspl42]

@rayane-djouah

I asked John to confirm the scope right after you were assigned, see: https://github.com/Expensify/App/issues/36166#issuecomment-1941575002

We agreed to fix "Empty state image has too much overlap" for "ALL room/chat types". It's my mistake that I missed this during the PR review. Sorry about that!

No, the solution to https://github.com/Expensify/App/issues/35921 is different

It doesn't matter; then our overall solution will have two parts. Please submit a follow-up PR with the second part of the solution! Thank you.

[rayane-djouah]

Sorry for the delay, missed this one.

I proposed to make the image height smaller in order to fit the area when we scroll a chat to the top.

current bug (a lot of space above the chat when we scroll to the top):

https://github.com/Expensify/App/assets/77965000/4488938a-9bc9-43dd-b3b8-07edeab5a49c

@shawnborton proposed this solution :

So my thinking is that it would be nice to treat the empty state like a background image of sorts. This way it doesn't actually take up a specified height. So if the viewport is quite tall and there isn't much content, we'd be able to see all of the empty state. But if there was a bunch of content in the viewport, then you'd only see like 20px of padding above the first chat/item in the view, rather than showing the whole image like we do today

if the viewport is quite tall and there isn't much content, we'd be able to see all of the empty state. But if there was a bunch of content in the viewport, then you'd only see like 20px of padding above the first chat/item in the view, rather than showing the whole image.

I think that this is already the situation today

This way it doesn't actually take up a specified height

I don't think that this is technically possible.

I inspected figma files and I found that recently we are using 337px on desktop and 230px for mobile for the image height.

In the app, it's set to 450px for desktop and 300px for mobile.

@Expensify/design could you confirm fix the expected result for the image height? Thank you!

cc @cubuspl42

[shawnborton]

Before we confirm the expected height across desktop/mobile, I would love to know if we even think it's technically possible to make that part of the chat view not scrollable.

If I were to do this in CSS, I think I would do something where I add the background empty state image to the very first message/item in the chat where it's basically absolutely positioned. And make sure the wrapper that wraps both the item's content and the background image doesn't have overflow: hidden. This way that first item always has the floating background image above it, but it's not counted as real estate that would eat up scrollable area in the entire chat view.

[rayane-djouah]

Thank you for the explanation! I will work on this tomorrow.

[rayane-djouah]

@shawnborton could you please review design changes in https://github.com/Expensify/App/pull/38449 ? Thank you.

[rayane-djouah]

Update: PR https://github.com/Expensify/App/pull/38449 in progress

[cubuspl42]

@shawnborton I know it's a bit late, but I'm having doubts whether I completely understand these words:

If I were to do this in CSS, I think I would do something where I add the background empty state image to the very first message/item in the chat where it's basically absolutely positioned. And make sure the wrapper that wraps both the item's content and the background image doesn't have overflow: hidden. This way that first item always has the floating background image above it, but it's not counted as real estate that would eat up scrollable area in the entire chat view.

While I share the basic intuition, I'm having trouble figuring out how this relates to scrollable content. It makes things somewhat more complex.

You said you have a vision of how to do this in CSS. Could you provide an extremely simplified HTML/CSS mockup? I mean the one where an "empty state image" is a colored rectangle, and the "messages" (things inside the scrollable wrapper) are some lorem ipsum blocks with a border or something like that. It would help me review the PR so I could ensure that what was implemented is what you had in mind.

[shawnborton]

Totally, I can make a quick demo with HTML/CSS. Will try to get to it later this afternoon/tonight.

[shawnborton]

@cubuspl42 can you try this CodePen here? https://codepen.io/shawnborton/pen/vYMpzEL

Zoom in and out with your browser to make the content area bigger/smaller, which should show you how this thing works with overflow.

Basically using the &::before selector and absolute positioning let's us add this red block to the yellow row, without making it add to the overall row height.

[cubuspl42]

Awesome!

I created a React Native variant of this demo.

https://github.com/Expensify/App/assets/2590174/65aaca05-6e79-45fd-9e6b-21649b72a608

@shawnborton Would you review the video above? Do things behave as they should?

@rayane-djouah Would you check out the code I shared? Does it help with understanding what we want to achieve layout-wise?

[shawnborton]

That is EXACTLY what I was thinking, looks great!

[rayane-djouah]

I reviewed the code you shared, and it clarifies the layout and functionality to be achieved. Thank you!

[melvin-bot[bot]]

Current assignee @rayane-djouah is eligible for the External assigner, not assigning anyone new.

[melvin-bot[bot]]

Upwork job price has been updated to $500

[melvin-bot[bot]]

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

[melvin-bot[bot]]

Upwork job price has been updated to $500

[mallenexpensify]

@allroundexperts reassigning, please take over as C+. If you don't have bandwidth, unassign yourself. Thanks PR is here:

• https://github.com/Expensify/App/pull/38449

[allroundexperts]

Thank you. I'll review this today & tomorrow.