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

feat: Add search message tab to the right sidebar #14043

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

JungleDruid
Copy link

@JungleDruid JungleDruid commented Dec 23, 2024

☑️ Resolves

🖌️ UI Checklist

🖼️ Screenshots / Screencasts

🏚️ Before 🏡 After
firefox_2024-12-23_13-51-21 firefox_2024-12-23_13-51-09
1.mp4

🚧 Tasks

  • ...

🏁 Checklist

  • 🌏 Tested with different browsers / clients:
    • Chromium (Chrome / Edge / Opera / Brave)
    • Firefox
    • Safari
    • Talk Desktop
    • Not risky to browser differences / client
  • 🖌️ Design was reviewed, approved or inspired by the design team
  • ⛑️ Tests are included or not possible
  • 📗 User documentation in https://github.com/nextcloud/documentation/tree/master/user_manual/talk has been updated or is not required

@JungleDruid JungleDruid marked this pull request as ready for review December 23, 2024 06:16
@Antreesy
Copy link
Contributor

Hi @JungleDruid, thanks for the contribution! That is a decent work, tried to play with it a bit 🚀

I assume it's meant to be used in Desktop client? cc @ShGKme for opinion

Couple of things to consider:
API-wise:

  • search is done via provider, but it originates from Talk backend anyway, I wonder if exposing it as Talk endpoint would affect performance to the better (not relevant for this PR, just a follow-up);
    Design-wise:
  • there were some discussions in past, that abusing right sidebar tabs is better to avoid (you have now 3 tabs on screenshots, during the call it will be 4 tabs, if breakout rooms are configured - 5!) I personally found the way Discord does it appropriate - replacing participants list with search results:
search users
image image
  • Filters could be wrapped in NcPopover (with some styling), similar way it's used in web Unified Search:
image

Frontend-wise:

  • We're now aiming to update old and write new Vue components with following structure and would greatly appreciate that from contributors:
<script setup lang="ts">
<template>
<style>
  • In addition to that, adding input/output typings in src/types/index.ts would be also nice, so component is aware, what to expect from this endpoint. (could be later improved, if endpoint will be exposed)
  • There is currently an issue with loading old message results (it pulls all messages until the end of convesation, which might freeze browser/client). But your PR reaches them correctly, so could be fixed separately, I'll try to take a look in it.

We might discuss it internally with the engineering and design teams only after holidays, so please don't rush with changes =) It is also possible to reach us via 💬 Talk team public 👥 channel, don't hesitate to ask there

@JungleDruid
Copy link
Author

Hey @Antreesy,

Thanks for the kind words and suggestions!

I originally tried adding the feature to the conversation search in the left sidebar, but it felt too crowded with all the different results. Adding filter options there didn’t seem to fit well either, so I decided to create a new tab instead. That said, I see your point, there are quite a few tabs now! 😅 I’ll explore the idea of replacing the right sidebar when search is activated (similar to Discord), or perhaps replacing the left sidebar (like Telegram Desktop).

For the filter styles, I did consider using NcPopover, but I thought keeping the filters visible might be more user-friendly since it wouldn’t disappear or block the view during searches. The design was inspired by VS Code:
Code_2024-12-23_18-57-56

I wasn’t aware of the Vue component structure shift, thanks for pointing that out! I’ll start using the new structure from now on.

Thanks again for taking the time to review and share your thoughts, and happy holidays! 🎄

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.

Search inside Talk App doesn't include talk messages
2 participants