Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add search box to chat overlay #30382

Merged
merged 5 commits into from
Oct 22, 2024
Merged

Conversation

Maks1mio
Copy link
Contributor

I've been wanting to suggest this for a long time, and even left a message in Figma saying:

image

In general, I propose a pull request. I think it could have been done better than what I did.

2024-10-22.012627.mp4

I think it'll be a good user experience.

Comment on lines 202 to 217
public void FilterChannels(string searchTerm)
{
foreach (var item in ItemFlow.Children)
{
if (string.IsNullOrEmpty(searchTerm))
{
item.Show();
continue;
}

if (item.Channel.Name.Contains(searchTerm, StringComparison.OrdinalIgnoreCase))
item.Show();
else
item.Hide();
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not be implementing search logic manually, should be using SearchContainer instead

Comment on lines 100 to 103
searchTextBox.OnUpdate += (newText) =>
{
privateChannelGroup.FilterChannels(searchTextBox.Text);
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not be doing things every frame, should be binding to changes of searchTextBox.Current instead

@bdach
Copy link
Collaborator

bdach commented Oct 22, 2024

@peppy code quality issues notwithstanding, any UX feedback on this? I'm not sure this should be limited to searching PM channels, I'd say the textbox should be topmost and allow searching public channels by name too?

@peppy
Copy link
Member

peppy commented Oct 22, 2024

@peppy code quality issues notwithstanding, any UX feedback on this? I'm not sure this should be limited to searching PM channels, I'd say the textbox should be topmost and allow searching public channels by name too?

Yep, sounds fine with your proposed change.

@Theighlin
Copy link

Would be neat if pressing enter automatically selects the first matching result (if it doesn't already)

@Maks1mio
Copy link
Contributor Author

When I'm at home, I'm be ready to edit the code

@bdach
Copy link
Collaborator

bdach commented Oct 22, 2024

I'm just gonna apply the changes myself to speed this up.

@bdach bdach self-assigned this Oct 22, 2024
@Maks1mio
Copy link
Contributor Author

ok thanks 😋

@bdach
Copy link
Collaborator

bdach commented Oct 22, 2024

After:

2024-10-22.12-33-35.mp4

@ppy/team-client requesting second review as I rewrote every line.

@bdach bdach requested a review from a team October 22, 2024 10:35
@bdach bdach changed the title Added search users by name in direct messages. Add search box to chat overlay Oct 22, 2024
@peppy peppy self-requested a review October 22, 2024 10:40
@peppy
Copy link
Member

peppy commented Oct 22, 2024

We probably need a PlatformAction.Search don't we 😅

@bdach
Copy link
Collaborator

bdach commented Oct 22, 2024

We probably need a PlatformAction.Search don't we 😅

Probably, but if you wanna go down that way we'd probably need to re-examine a bunch of UX (mod select comes to mind first and foremost?)

@peppy
Copy link
Member

peppy commented Oct 22, 2024

Can happen later, just feels like i should be able to focus search here quickly

@peppy peppy merged commit 213be02 into ppy:master Oct 22, 2024
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants