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! 🎄

Copy link
Contributor

github-actions bot commented Jan 7, 2025

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

@nimishavijay
Copy link
Member

Awesome work @JungleDruid ! :) From a design perspective it looks really nice! Here are some points that would make it integrate better with Nextcloud's general design

  • Like @Antreesy said, we try to not add more tabs in the sidebar so
    • the entry point can be an icon-only search button left of the close button, instead of a tab
    • clicking on it would replace the contents of the sidebar (including the tabs) with search, and replace the conversation name on the top of the sidebar with "Search in [conversation name]" with a back button on the left which would exit the search
  • As for the filters, we could take inspiration from the filtering in Files or unified search and use NcUserBubble component to show the applied filters.
    • If that's not possible, I strongly suggest changing the icon to a filter Material Design symbol and keeping the height constant -- buttons with height more than width is not something we see often in Nextcloud apps. The button can always be on the same row as the search field
  • Another thing to keep in mind is the behaviour when we are in a call, as the chat is also in the sidebar
    • In this case, the search will replace the chat, so clicking on a message would replace the search with the chat again, and highlight the message
    • It would also be nice if when we went back to the search, the state of the search is saved (for the rather common usecase that we searched for a message and clicked on the wrong one and went back to click on the right one)

Thanks for amazing contribution! Let me know what you think about the design feedback :)

@Antreesy
Copy link
Contributor

Antreesy commented Jan 9, 2025

In addition to design comments, and for information @JungleDruid:

We are currently preparing 31 beta release and would like to get this feature in the nearest time (so it appears in Desktop client shortly).
Migration to Vue3 + Typescript is done nicely, but I would add some comments regarding types in the project structure, and some other minor things, e.t.c.
If you feel like you don't have a time to address this shortly, we could take over and finish this PR (on top of your commits with keeping authorship). Tell us what, do you think?

@JungleDruid
Copy link
Author

Hi @Antreesy,

Unfortunately, I’m tied up with another project and won’t be able to address the changes soon. Please feel free to take over and complete the PR. Let me know if there’s anything you need from me.

Thanks!

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
4 participants