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.44k stars 2.8k forks source link

Chat - There is no member search for a group chat with more than 8 members #41909

Open lanitochka17 opened 4 months ago

lanitochka17 commented 4 months 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: 1.4.72.0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4548098&group_by=cases:section_id&group_id=296760&group_order=asc Issue reported by: Applause - Internal Team

Action Performed:

  1. Log in with an expensifail account
  2. Create a group chat with more than 8 members
  3. Navigate to the members tab of the group chat

Expected Result:

There should be a member search

Actual Result:

There is no member search for a group chat with more than 8 members

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/32ea58bf-1cbc-44c1-a01f-d06f89818283

View all open jobs on GitHub

melvin-bot[bot] commented 4 months ago

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

lanitochka17 commented 4 months ago

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

lanitochka17 commented 4 months ago

We think that this bug might be related to #vip-vsp

Krishna2323 commented 4 months ago

Proposal

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

Chat - There is no member search for a group chat with more than 8 members

What is the root cause of that problem?

New feature

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

  1. We need to create a state for searchInput and pass the input value to textInputValue and also pass onChangeText for updating the searchInput.
  2. Then pass the textInputLabel according to if participants length is >8.
  3. In getUsers function filter out the participant which does not match the searchInput.
  4. We also need to styles the input, to place it below the text headerContent.

For proper filtering we can follow getOptions util function in OptionsListUtils. https://github.com/Expensify/App/blob/807c945ef8edabe86cc62f16c866aa754db8750b/src/libs/OptionsListUtils.ts#L1639

What alternative solutions did you explore? (Optional)

Result

https://github.com/Expensify/App/assets/85894871/716f2f42-7224-40ce-825f-d250154818b7

Krishna2323 commented 4 months ago

Proposal Updated

nkdengineer commented 4 months ago

Proposal

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

There is no member search for a group chat with more than 8 members

What is the root cause of that problem?

This is a new feature

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

  1. Create a searchTerm state to control the value of search input
const [searchValue, setSearchValue] = useState<string>('');
  1. Add textInputLabel prop into SelectionList, it will be empty if the members <=8 otherwise it's a string that can be confirm by design team
textInputLabel={chatParticipants.length > 8 ? translate('optionsSelector.findMember') : ''}
textInputValue={searchValue}
onChangeText={setSearchValue}

https://github.com/Expensify/App/blob/db469d6e93e8eeae8c1251d1ebd9a94a6dfdd502/src/pages/ReportParticipantsPage.tsx#L347

  1. In getUsers function, add dependency searchValue and we will get the members that match with searchValue. To filter the members, we can use the same logic as we do in RoomMemberPage here
// If search value is provided, filter out members that don't match the search value
if (searchValue.trim()) {
    let memberDetails = '';
    if (details.login) {
        memberDetails += ` ${details.login.toLowerCase()}`;
    }
    if (details.firstName) {
        memberDetails += ` ${details.firstName.toLowerCase()}`;
    }
    if (details.lastName) {
        memberDetails += ` ${details.lastName.toLowerCase()}`;
    }
    if (details.displayName) {
        memberDetails += ` ${PersonalDetailsUtils.getDisplayNameOrDefault(details).toLowerCase()}`;
    }
    if (details.phoneNumber) {
        memberDetails += ` ${details.phoneNumber.toLowerCase()}`;
    }

    if (!OptionsListUtils.isSearchStringMatch(searchValue.trim(), memberDetails)) {
        return;
    }
}

https://github.com/Expensify/App/blob/db469d6e93e8eeae8c1251d1ebd9a94a6dfdd502/src/pages/ReportParticipantsPage.tsx#L71

  1. OPTIONAL: We can also add headerMessage into SelectionList if there's no member match with searchValue and hide the headerContent as well. We can discuss more about this on the PR phrase
const headerMessage = searchValue.trim() && !participants.length ? translate('roomMembersPage.memberNotFound') : '';
headerMessage={headerMessage}

What alternative solutions did you explore? (Optional)

NA

Result

https://github.com/Expensify/App/assets/161821005/5ecac80d-3d66-42fa-b3f7-7e9a72a051fe

melvin-bot[bot] commented 4 months ago

@muttmuure Eep! 4 days overdue now. Issues have feelings too...

muttmuure commented 4 months ago

Handling today

muttmuure commented 4 months ago

This would be a new feature, it is not a bug

muttmuure commented 4 months ago

https://expensify.slack.com/archives/C05RECHFBEW/p1715779669595809

melvin-bot[bot] commented 4 months ago

Current assignee @muttmuure is eligible for the NewFeature assigner, not assigning anyone new.

melvin-bot[bot] commented 4 months ago

:warning: It looks like this issue is labelled as a New Feature but not tied to any GitHub Project. Keep in mind that all new features should be tied to GitHub Projects in order to properly track external CAP software time :warning:

melvin-bot[bot] commented 4 months ago

Triggered auto assignment to Design team member for new feature review - @dubielzyk-expensify (NewFeature)

dubielzyk-expensify commented 4 months ago

Is this something we actually want @Expensify/design ? If so, I suspect we'd do something like this?

CleanShot 2024-05-17 at 09 44 30@2x

shawnborton commented 4 months ago

Hmm interesting. I know that when we do selection lists, if there are more than 8 items in the list, that's when we add the search box: CleanShot 2024-05-16 at 21 08 11@2x

However, I'm not sure that we ever discussed how to handle this with our table views. I know we had punted adding any kind of search bar on top of the table view in the Workspace editor until we fleshed out the global search router idea some more. Is that right @JmillsExpensify @trjExpensify?

That being said, @dubielzyk-expensify I do really love what you have here, but I guess we should first figure out how this interacts with the router. Given that this "View Members" view would always show up in the RHP, and would essentially sit on top of the router, I feel like it would be fine to have an encapsulated search box here?

dubielzyk-expensify commented 4 months ago

Yeah. I remember us exploring it briefly. We also have search in Rooms already:

CleanShot 2024-05-17 at 11 14 59@2x

Which brings up a side-note that the Members page on Groups and Rooms should probably be consistent.

shawnborton commented 4 months ago

Totally agree. Yeah, I think with the new Groups implementation, @marcaaron implemented a different kind of table view than the old one we had for rooms. Any thoughts on that Marc? I agree though, we should make them consistent.

marcaaron commented 4 months ago

I definitely agree with making the Room and Group Chat members view consistent. I also really like what @dubielzyk-expensify shared above.

@shawnborton you make a good point about the two step router. Would we need something like that on the "Members" pages? If not, then @dubielzyk-expensify's mockup looks perfect to me and we should align Group Chats and Rooms to both use that.

Otherwise, I'd worry we might end up with one too many search icons in this view. I don't have any great ideas for what that two-step router would do on this page. So, I think I'd lean towards leaving it out entirely and go with the inline filter instead.

shawnborton commented 4 months ago

Yeah, I like that idea too personally - I do think that even in a world with a router, this kind of RHP would fly open and sit on top of the router so it's totally cool if we just have a simple search input above the table that filters the current table you are looking at.

dannymcclain commented 4 months ago

Totally agree with where you all are headed with this!

trjExpensify commented 4 months ago

However, I'm not sure that we ever discussed how to handle this with our table views. I know we had punted adding any kind of search bar on top of the table view in the Workspace editor until we fleshed out the global search router idea some more. Is that right @JmillsExpensify @trjExpensify?

That's right yeah, we've punted adding additional search inputs above the table views in settings (i.e Categories, Members, Tags etc) until we get the router out the door.

muttmuure commented 3 months ago

Future issue

dubielzyk-expensify commented 2 months ago

Any updates on this?

dubielzyk-expensify commented 1 month ago

Any updates?

muttmuure commented 1 month ago

Who is that question for @dubielzyk-expensify? I don't think anyone is working on this

dubielzyk-expensify commented 1 month ago

I'm just asking cause Melvin keeps saying it's overdue. Shall we close this or is it still being planned at some stage?

dubielzyk-expensify commented 3 days ago

Same as above