Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.36k stars 2.78k forks source link

[$250] [Search v2.3] - Default name is not displayed correctly for multiline system message thread #49330

Open IuliiaHerets opened 1 week ago

IuliiaHerets commented 1 week 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: 9.0.36-0 Reproducible in staging?: Y Reproducible in production?: Y Email or phone of affected tester (no customers): applausetester+kh010901@applause.expensifail.com Issue reported by: Applause Internal Team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go to DM.
  3. Submit an expense with a multiline description.
  4. Go to transaction thread.
  5. Click Description.
  6. Edit the description while maintaining the multiline format and save it.
  7. Right click on the system message > Reply in thread.
  8. Send a reply in thread.
  9. Go to Search > Chats.
  10. Click Filters > Select "In".
  11. Select the thread in Step 8 > Save.
  12. Click Save search.

Expected Result:

The title of the saved search should only have one line.

Actual Result:

The title of the saved search has multiple lines and it is not displayed correctly.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

https://github.com/user-attachments/assets/ada2fbd2-fe7c-442a-a7f1-080bbdbbaa1b

Bug6606270_1726554285396!Screenshot_2024-09-17_at_14 22 04

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021836683698134866207
  • Upwork Job ID: 1836683698134866207
  • Last Price Increase: 2024-09-19
  • Automatic offers:
    • paultsimura | Reviewer | 104047003
    • Krishna2323 | Contributor | 104047004
Issue OwnerCurrent Issue Owner: @paultsimura
melvin-bot[bot] commented 1 week ago

Triggered auto assignment to @abekkala (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.

IuliiaHerets commented 1 week ago

We think that this bug might be related to #wave-control

IuliiaHerets commented 1 week ago

@abekkala 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

CyberAndrii commented 1 week ago

Proposal

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

Saved search - Default name is not displayed correctly for multiline system message thread

What is the root cause of that problem?

The title text is not wrapped correctly.

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

Add titleStyle: styles.textNoWrap, here

https://github.com/Expensify/App/blob/14b99ca0a12e9686818bc3e937f091199de69750/src/pages/Search/SearchTypeMenu.tsx#L110

drminh2807 commented 1 week ago

Proposal

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

Saved search - Default name is not displayed correctly for multiline system message thread

What is the root cause of that problem?

The title text is not wrapped correctly.

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

add numberOfLinesTitle: 2, in https://github.com/Expensify/App/blob/14b99ca0a12e9686818bc3e937f091199de69750/src/pages/Search/SearchTypeMenu.tsx#L108-L117

What alternative solutions did you explore? (Optional)

add noWrap style titleStyle: styles.textNoWrap,

Krishna2323 commented 1 week ago

Proposal


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

Saved search - Default name is not displayed correctly for multiline system message thread

What is the root cause of that problem?

The default value for numberOfLinesTitle is 1 but the text overflows because we are applying styles.pre instead of styles.noWrap. https://github.com/Expensify/App/blob/2190f6279041ed8260752294d095bbdda76faebe/src/components/MenuItem.tsx#L350 https://github.com/Expensify/App/blob/2190f6279041ed8260752294d095bbdda76faebe/src/components/MenuItem.tsx#L444-L457

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


We should update the condition below to numberOfLinesTitle !== 1 ? styles.preWrap : styles.noWrap,. https://github.com/Expensify/App/blob/2190f6279041ed8260752294d095bbdda76faebe/src/components/MenuItem.tsx#L451

What alternative solutions did you explore? (Optional)

Result

truph01 commented 1 week ago

Proposal

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

What is the root cause of that problem?

so the saved search name can contain multiple lines.

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

https://github.com/Expensify/App/blob/7bd9e53edfd2508dd4584e1db97fbf7a69f8a646/src/libs/SearchUtils.ts#L715

so it can be:

        return ReportUtils.formatReportLastMessageText(ReportUtils.getReportName(reports?.[`${ONYXKEYS.COLLECTION.REPORT}${filter}`]));

like we did in this:

https://github.com/Expensify/App/blob/7bd9e53edfd2508dd4584e1db97fbf7a69f8a646/src/components/DisplayNames/DisplayNamesWithTooltip.tsx#L59

What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 1 week ago

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

melvin-bot[bot] commented 1 week ago

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

paultsimura commented 1 week ago

Reviewing now 👀

paultsimura commented 1 week ago

The proposal by @CyberAndrii is the least invasive while it solves the issue correctly, so I'd go with it.

Proposals to add numberOfLinesTitle: 2 also do the trick, but it doesn't match the expected behavior that requires the item to contain only 1 line.

🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 1 week ago

Current assignees @luacmartins and @lakchote are eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

Krishna2323 commented 1 week ago

@paultsimura, my proposal here solves the root cause. Could you please check again?

paultsimura commented 1 week ago

On a side note, @Krishna2323 please focus on finishing your open PRs instead of posting new proposals as the contribution guideline requires.

paultsimura commented 1 week ago

@paultsimura, my proposal here solves the root cause. Could you please check again?

Your proposal is very invasive, it suggests replacing the formatting for all the MenuItem components, which can cause regressions.

@CyberAndrii any chance you could find an example where using this proposal breaks styling in other places>

truph01 commented 1 week ago

@paultsimura What do you think about my solution, which just needs to use ReportUtils.formatReportLastMessageText without any style changes? Thank you.

truph01 commented 1 week ago

I tried to test your solution but It does not fix in case of IOS safari. Can you help confirm if I was wrong? @CyberAndrii

Krishna2323 commented 1 week ago

@paultsimura, I don't think it will cause any regression because we will be only applying styles.noWrap when numberOfLines is 1.

paultsimura commented 1 week ago

Valid point @truph01. Taking another look at the proposals now...

paultsimura commented 1 week ago

Ok, after another look, I tend to agree with @Krishna2323's proposal.

It makes sense to have noWrap where numberOfLines: 1. However, to be on the safe side, we'll have to go through the places that use MenuItem component with numberOfLines: 1 (or no value provided, meaning it uses the default – still 1), and check if they really are meant to have numberOfLines: 1 or if we should update them by specifying the correct numberOfLines.

🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 1 week ago

Current assignees @luacmartins and @lakchote are eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

truph01 commented 1 week ago

@paultsimura Please note we need to fix the search header page with that solution as well:

image

paultsimura commented 1 week ago

@paultsimura Please note we need to fix the search header page with that solution as well:

image

There is no such requirement. From what I see, the search header is explicitly set to have numberOfLines: 2. If the design team requests so, we can change it to numberOfLines: 1.

https://github.com/Expensify/App/blob/14f952a59de4968699e92541f3b326f4d09e30ad/src/components/Search/SearchPageHeader.tsx#L59-L66

melvin-bot[bot] commented 1 week ago

📣 @paultsimura 🎉 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 week ago

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

paultsimura commented 1 week ago

@Expensify/design should we make the search header subtitle 1-lined with no wrap, or keep it as is (currently it's 2 lines)? image

image
shawnborton commented 1 week ago

It should only be 1 line and then truncated but why is it not occupying more horizontal space? There's a lot more space over on the right that it could be using.

paultsimura commented 1 week ago

why is it not occupying more horizontal space?

That's because the text contains line breaks that are preserved. Changing numberOfLines to 1 resolves it:

image

@Krishna2323 please consider this in your PR.

shawnborton commented 1 week ago

Ah nice, that's much better

Krishna2323 commented 1 week ago

@Krishna2323 please consider this in your PR.

Yeah, sure.

Krishna2323 commented 1 week ago

@paultsimura, PR ready for review ^

luacmartins commented 3 days ago

PR merged