App icon indicating copy to clipboard operation
App copied to clipboard

Search - Display names are bold

Open lanitochka17 opened this issue 1 year ago • 5 comments

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 1.4.38-0 Reproducible in staging?: Y Reproducible in production?: N If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): [email protected] Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: Applause - Internal Team Slack conversation:

Action Performed:

  1. Navigate to staging.new.expensify.com
  2. Click on the search bar

Expected Result:

The display name of the chats should not be bold

Actual Result:

The display name of the chats is bold

Workaround:

Ignore it

Platforms:

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

  • [x] Android: Native
  • [x] Android: mWeb Chrome
  • [x] iOS: Native
  • [ ] iOS: mWeb Safari
  • [x] MacOS: Chrome / Safari
  • [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Staging

https://github.com/Expensify/App/assets/78819774/197cad8b-5d6b-47a1-a275-7953827a6b36

Production

https://github.com/Expensify/App/assets/26260477/c0f03ebf-3148-4f6d-a107-2000114a0f6f

View all open jobs on GitHub

lanitochka17 avatar Feb 07 '24 20:02 lanitochka17

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

github-actions[bot] avatar Feb 07 '24 20:02 github-actions[bot]

Triggered auto assignment to @neil-marcellini (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

melvin-bot[bot] avatar Feb 07 '24 20:02 melvin-bot[bot]

Interesting, I wouldn't think this would be a blocker, it's been like this since we first created NewDot

shawnborton avatar Feb 07 '24 21:02 shawnborton

The display name is bold in search regardless of the unread status of that chat, so I don't understand why that's part of the test steps.

I think there is a small regression here, but it's very different than what's described in the current issue.

Here's the current behavior on prod

https://github.com/Expensify/App/assets/26260477/c0f03ebf-3148-4f6d-a107-2000114a0f6f

I will update the issue from what's currently described to something that's more clear.

Currently:

Action Performed:

  1. Navigate to staging.new.expensify.com
  2. Click on any unread message
  3. Click on other chat so the chat from step 2 is not bold anymore
  4. Navigate to Search

Expected Result:

Since the chat is open once the Display name of the sender should not be bold anymore

Actual Result:

Display name remains bold in search when user open the conversation

New bug description:

Action Performed:

  1. Navigate to staging.new.expensify.com
  2. Click on the search bar

Expected Result:

The display name of the chats should not be bold

Actual Result:

The display name of the chats is bold

Furthermore, this is a very minor regression, so I won't consider it a blocker.

neil-marcellini avatar Feb 07 '24 22:02 neil-marcellini

It would make more sense if the style of the chats on the search page matched the LHN. That's not the case on staging or prod. What do you think @shawnborton?

neil-marcellini avatar Feb 07 '24 22:02 neil-marcellini

Proposal

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

The display name on the search page is bold.

What is the root cause of that problem?

This happened after the search page used SelectionList (in https://github.com/Expensify/App/pull/35058). The selection list item has a bold style by default. https://github.com/Expensify/App/blob/9f37d3e2c1264420773c886ae6171748e5d512b8/src/components/SelectionList/BaseListItem.tsx#L108

This can be seen on some page that already uses a selection list, such as the room member page. image

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

If we want to bold it only when the chat is unread, we can add a new attribute to the list item called isBold and only apply the bold style if isBold is not false.

item.isBold !== false && styles.sidebarLinkTextBold,

Then, we need to map the report array here to include isBold attribute https://github.com/Expensify/App/blob/9f37d3e2c1264420773c886ae6171748e5d512b8/src/pages/SearchPage/index.js#L95

data: recentReports.map(report => ({...report, isBold: report.isUnread})),

What alternative solutions did you explore? (Optional)

If we want to disable the bold for all items, we can add a new props to the SelectionList itself to disable the bold style.

Or if we don't want to bold a list item that is a user list item, then we can apply the bold style only if it's not a user list item. !isUserItem ? styles.sidebarLinkTextBold : null,

bernhardoj avatar Feb 08 '24 05:02 bernhardoj

@trjExpensify or @JmillsExpensify or @Expensify/design do you have any context on this one?

As far as I know, results in the chat search list were always bold. And this is because that's our standard pattern for displaying this kind of information in a list.

Then at some point, we made the workspace switcher use this bold or non-bold pattern depending on if there was unread messages or not.

I think for the chat switcher, it does make sense that the chat matches how it would look in the LHN. So if the chat is unread, keep it bold. If the chat is read, make it not-bold.

But for things like the flow to start a new chat, or looking at a list of members, etc - I think we should keep our current pattern and use bold for the names.

Let me know if all of that makes sense!

shawnborton avatar Feb 08 '24 13:02 shawnborton

☝️ I agree with everything you said.

I also thought they were always bold and was surprised to find out that's not the case!

I'm actually torn on whether or not we should make the chats match the LHN or just have them all bold all the time. Reason being is that it's kinda weird that a new chat with someone you've never chatted with would be bold (denoting it has unread messages). So I wonder if by trying to include that we're actually just doing too much and making it more complicated than it needs to be. And to be clear, I think the workspace switcher is a totally different type of searching/switching and the pattern we have there still makes sense.

dannymcclain avatar Feb 08 '24 14:02 dannymcclain

Reason being is that it's kinda weird that a new chat with someone you've never chatted with would be bold (denoting it has unread messages).

Totally fair, but I think of that as something that actually doesn't exist in your LHN, and thus would just follow the standard list option patterns we have?

shawnborton avatar Feb 08 '24 15:02 shawnborton

Yeah that's a good point. Totally down for what you're proposing!

dannymcclain avatar Feb 08 '24 15:02 dannymcclain

I think for the chat switcher, it does make sense that the chat matches how it would look in the LHN. So if the chat is unread, keep it bold. If the chat is read, make it not-bold.

But for things like the flow to start a new chat, or looking at a list of members, etc - I think we should keep our current pattern and use bold for the names.

@shawnborton I totally agree with these statements ^

a new chat with someone you've never chatted with would be bold (denoting it has unread messages).

@dannymcclain I agree that if you search for a user that you have never chatted with, it's odd to show that new entry as bold. I think we should make this not bold so it appears that you don't have any unread messages from them, which is true given that it's a new chat. I see Shawn's point too, but if we're going to make this list of chats match the LHN state, then I think we have to stay consistent there for it to make sense.

neil-marcellini avatar Feb 08 '24 20:02 neil-marcellini

Hmm but it wouldn't appear in your LHN like that. So I still think for the create case, we should mimic what we do for our generic list/option views where the title is bold. Because in your LHN, it actually looks something more like this (it has the preview line): CleanShot 2024-02-08 at 15 16 01@2x

shawnborton avatar Feb 08 '24 20:02 shawnborton

I think because they're different contexts they don't have to follow the same pattern. I kinda lean towards bold for the reasons @shawnborton mentioned.

dubielzyk-expensify avatar Feb 08 '24 23:02 dubielzyk-expensify

@neil-marcellini Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] avatar Feb 12 '24 15:02 melvin-bot[bot]

Ok so it sounds like this is the desired behavior. I'm going to update the issue description and send it to be implemented externally.

  • Chats on the search page should display with their name bold when unread, and not bold otherwise, to match the chat list in the LHN
  • Searching for a new chat should display the name in bold
  • For the flow to start a new chat, or looking at a list of members, etc, use bold for the names

neil-marcellini avatar Feb 12 '24 19:02 neil-marcellini

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

melvin-bot[bot] avatar Feb 12 '24 20:02 melvin-bot[bot]

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

melvin-bot[bot] avatar Feb 12 '24 20:02 melvin-bot[bot]

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

melvin-bot[bot] avatar Feb 12 '24 20:02 melvin-bot[bot]

nice! Let the proposals roll in!

dylanexpensify avatar Feb 13 '24 20:02 dylanexpensify

Proposal

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

Search - Display names are bold regardless of unread state

What is the root cause of that problem?

See

  • pages/SearchPage/index.js
  • components/SelectionList/BaseSelectionList.tsx
  • components/SelectionList/BaseListItem.tsx

SearchPage includes BaseSelectionList which defines renderItem function. That function returns BaseListItem. BaseListItem always includes the style styles.sidebarLinkTextBold.

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

See

  • components/LHNOptionsList/OptionRowLHN.tsx

In BaseListItem, include a prop named something like alwaysBold and set it to true by default.

In BaseListItem, if alwaysBold is false, use the same logic as is used in OptionRowLHN to determine whether or not to use bold. That logic is found in definition of textUnreadStyle.

SearchPage is not the only component to include BaseSelectionList. Therefore, in BaseSelectionList also include an alwaysBold prop with default value of true and pass it to BaseListItem. In SearchPage, send alwaysBold={false} to BaseSelectionList.

kmbcook avatar Feb 13 '24 22:02 kmbcook

As per the implementation requirements here https://github.com/Expensify/App/issues/36075#issuecomment-1939464168 , @bernhardoj 's proposal here https://github.com/Expensify/App/issues/36075#issuecomment-1933392317 looks good and works well. It can be little modified to match all the requirements which we could take care in PR state and it is also the first proposal.

🎀 👀 🎀 C+ Reviewed

abdulrahuman5196 avatar Feb 16 '24 08:02 abdulrahuman5196

Current assignee @neil-marcellini is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

melvin-bot[bot] avatar Feb 16 '24 08:02 melvin-bot[bot]

@neil-marcellini, @abdulrahuman5196, @dylanexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] avatar Feb 19 '24 15:02 melvin-bot[bot]

@neil-marcellini mind confirming proposal is acceptable?

dylanexpensify avatar Feb 20 '24 09:02 dylanexpensify

@neil-marcellini @abdulrahuman5196 @dylanexpensify this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] avatar Feb 21 '24 15:02 melvin-bot[bot]

@neil-marcellini Bump on this?

abdulrahuman5196 avatar Feb 22 '24 13:02 abdulrahuman5196

DM'ing Neil!

dylanexpensify avatar Feb 26 '24 11:02 dylanexpensify

Sorry for the delay, looking now

neil-marcellini avatar Feb 26 '24 22:02 neil-marcellini

As per the implementation requirements here #36075 (comment) , @bernhardoj 's proposal here #36075 (comment) looks good and works well. It can be little modified to match all the requirements which we could take care in PR state and it is also the first proposal.

🎀 👀 🎀 C+ Reviewed

I agree, hiring!

neil-marcellini avatar Feb 26 '24 22:02 neil-marcellini

📣 @abdulrahuman5196 🎉 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] avatar Feb 26 '24 22:02 melvin-bot[bot]