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.49k stars 2.85k forks source link

[PAY 15th SEP][$250] Member names in member selection views are not bold or using correct text color and styles #48364

Closed m-natarajan closed 1 month ago

m-natarajan commented 1 month 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: Reproducible in staging?: Needs reproduction Reproducible in production?: Needs reproduction 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/p1724943353451769

Action Performed:

  1. Go to staging.new.expensify.com
  2. Initiate an IOU

    Expected Result:

    Member names in member selection views are bold and using correct text color and styles

    Actual Result:

    Member names in member selection views are not bold or using correct text color and styles

    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

CleanShot 2024-08-29 at 10 55 02@2x

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0159f84c9f700169cc
  • Upwork Job ID: 1830998979182407156
  • Last Price Increase: 2024-09-03
Issue OwnerCurrent Issue Owner: @jayeshmangwani
melvin-bot[bot] commented 1 month ago

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

MelvinBot commented 1 month ago

This has been labelled "Needs Reproduction". Follow the steps here: https://stackoverflowteams.com/c/expensify/questions/16989

FitseTLT commented 1 month ago

@shawnborton the option will be bold when the report linked to it is unread. This is intentional. If I have understood the issue. 🤷

bernhardoj commented 1 month ago

Proposal

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

The money request participant title isn't using bold style.

What is the root cause of that problem?

The title text will be bold if the isBold property of the item is not false. https://github.com/Expensify/App/blob/117b96112b4b2de8ead192c9c1da619edb9860fa/src/components/SelectionList/InviteMemberListItem.tsx#L101

The isBold logic was first added in https://github.com/Expensify/App/pull/37306 and we want all list by default has a bold text, except for LHN and chat finder page where it will be bold based on the unread status.

Previously, isBold was only added to ChatFinderPage list, but now, we added it to all lists by adding it in getOptions. https://github.com/Expensify/App/blob/117b96112b4b2de8ead192c9c1da619edb9860fa/src/libs/OptionsListUtils.ts#L1968

So, all user lists will follow the LHN unread logic to bold the title, including personal details which don't have an unread status, so it will always be false. https://github.com/Expensify/App/blob/117b96112b4b2de8ead192c9c1da619edb9860fa/src/libs/OptionsListUtils.ts#L1992

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

We can add a new param to getOptions called shouldBoldTitleByDefault which has a default value of true. Then, pass it as false from getSearchOptions.

The new isBold condition will be.

reportOption.isBold = shouldBoldTitleByDefault || shouldUseBoldText(reportOption);

We can do the same for the personal details, but since shouldUseBoldText is always false for personal detail, we can just simplify the condition to: https://github.com/Expensify/App/blob/117b96112b4b2de8ead192c9c1da619edb9860fa/src/libs/OptionsListUtils.ts#L1992

personalDetailOption.isBold = shouldBoldTitleByDefault;

Or just remove it so personal detail will always be in bold just like before in https://github.com/Expensify/App/pull/37306, even in chat finder page.

shawnborton commented 1 month ago

the option will be bold when the report linked to it is unread. This is intentional. If I have understood the issue. 🤷

I think this is a bug. We only want the read/unread styles to be applied when you use the Find chat page. Otherwise any time we just list out members/contacts, the names should be bold and in our normal text color. cc @Expensify/design @trjExpensify @JmillsExpensify for the gut check there.

dannymcclain commented 1 month ago

@shawnborton definitely agree.

trjExpensify commented 1 month ago

We only want the read/unread styles to be applied when you use the Find chat page

why do I have a recollection that we decided to not use bold for unread at all on this page, did we walk back from that? 🤔

shawnborton commented 1 month ago

I feel like I recall a similar conversation but I had the same exact opinion/feedback as I do today and I could have sworn we implemented it correctly, hence why I was pretty convinced this was a regression.

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

jayeshmangwani commented 1 month ago

@bernhardoj since we determine the bold text status from the shouldUseBoldText fucntion's output, what do you think about making default bold text changes to the shouldUseBoldText function?

bernhardoj commented 1 month ago

Hmm, I think it's a bit weird to see the function accepting a boolean just to return the boolean.

function shouldUseBoldText(report, shouldBoldTitleByDefault) {
    return shouldBoldTitleByDefault || (report.isUnread === true && report.notificationPreference !== CONST.REPORT.NOTIFICATION_PREFERENCE.MUTE);
}

shouldUseBoldText(report, shouldBoldTitleByDefault);

I prefer to keep shouldBoldTitleByDefault outside the function.

jayeshmangwani commented 1 month ago

I prefer to keep shouldBoldTitleByDefault outside the function.

Thanks for the explanation. I was thinking about using it in the function itself, but I also think we can keep the default bold parameter outside.

jayeshmangwani commented 1 month ago

We can go with @bernhardoj 's Proposal of keeping bold text default to true and making it false for search options using the new parameter.

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 1 month ago

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

bernhardoj commented 1 month ago

PR is ready

cc: @jayeshmangwani

bfitzexpensify commented 1 month ago

I am heading out of office until September 21st, so assigning a buddy to watch over this in my absence.

Current status: PR in review

melvin-bot[bot] commented 1 month ago

Current assignee @bfitzexpensify is eligible for the Bug assigner, not assigning anyone new.

melvin-bot[bot] commented 1 month ago

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

jayeshmangwani commented 1 month ago

@jliexpensify Automation failed here; PR was deployed to production 4 days ago with the following checklist https://github.com/Expensify/App/issues/48664

jliexpensify commented 1 month ago

Thanks @jayeshmangwani - will note it down!

jliexpensify commented 1 month ago

Payment Summary

Is a checklist needed?

jayeshmangwani commented 1 month ago

Is a checklist needed?

Yes, I'll complete the checklist today

bernhardoj commented 1 month ago

Requested in ND.

jayeshmangwani commented 1 month ago

Regression Test Proposal

  1. Open App, Press FAB and select Submit Expense.
  2. Enter any amount and proceed to the next step.
  3. Verify that all member names in the member selection view are displayed in bold.

Do we agree 👍 or 👎

jliexpensify commented 1 month ago

Cheers, closing!

jayeshmangwani commented 1 month ago

Requested as per https://github.com/Expensify/App/issues/48364#issuecomment-2348547465

JmillsExpensify commented 1 month ago

$250 approved for @bernhardoj

garrettmknight commented 1 month ago

$250 approved for @jayeshmangwani