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

[Feature] Add basic functionality to the mention_input_bar component #372

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

Conversation

ZhangHanDong
Copy link
Contributor

@ZhangHanDong ZhangHanDong commented Feb 1, 2025

Add basic functionality to the mention_input_bar component. For issue #278

The mention_input_bar component requires these modifications to the CommandTextInput component of Makepad to work :Makepad PR #651

2
3

TODO for the future:

  1. Add a header to the user list to display the current number of users in the room.
  2. Implement scrolling functionality for the user list.
  3. Enable sorting for the user list to show currently online users.
  4. Optimize performance and add a loading animation for the user list.

@ZhangHanDong
Copy link
Contributor Author

The current error in CI with cargo check is because the dependent CommandTextInput component has not been updated to the latest version.
This PR needs to be merged. Makepad PR #651

@ZhangHanDong ZhangHanDong added the waiting-on-review This issue is waiting to be reviewed label Feb 6, 2025
@alanpoon
Copy link
Contributor

Screenshot 2025-02-10 at 3 19 16 PM

  1. Mentionable List should not appear when only the "@" is being typed, it should appear only after "@ and some match characters"
  2. There should be a header "Users" at the top of the mention-able List
  3. Mentionable List seems to contain current user and does not contain one remaining room user.

mention2
The typing textbox needs to also be able to show avatar, after selecting the mentioned user.
5.
mention3
The typing textbox is not able to send the correct format for the mentioning the user. After mentioning the user, the message should show a hyperlink for the mentioned user.

https://github.com/user-attachments/assets/9f43e74e-7f0d-4ed6-a69c-322396f5a2af
The highlighted background by the mouse should be same color as default highlighted mentionable user/

  1. In the mentionable List, the user_id should have a "@" at the front.

@ZhangHanDong ZhangHanDong added waiting-on-author This issue is waiting on the original author for a response and removed waiting-on-review This issue is waiting to be reviewed labels Feb 11, 2025
@ZhangHanDong
Copy link
Contributor Author

ZhangHanDong commented Feb 11, 2025

Screenshot 2025-02-10 at 3 19 16 PM

  1. Mentionable List should not appear when only the "@" is being typed, it should appear only after "@ and some match characters"
  2. There should be a header "Users" at the top of the mention-able List
  3. Mentionable List seems to contain current user and does not contain one remaining room user.

mention2 The typing textbox needs to also be able to show avatar, after selecting the mentioned user. 5. mention3 The typing textbox is not able to send the correct format for the mentioning the user. After mentioning the user, the message should show a hyperlink for the mentioned user.

https://github.com/user-attachments/assets/9f43e74e-7f0d-4ed6-a69c-322396f5a2af The highlighted background by the mouse should be same color as default highlighted mentionable user/

  1. In the mentionable List, the user_id should have a "@" at the front.

I don’t quite understand the reason for the first point.

I will continue to address the suggestions 2/6/7.

The others can be considered as features to be added in the future. This is just the initial version.

@kevinaboos
Copy link
Member

The current error in CI with cargo check is because the dependent CommandTextInput component has not been updated to the latest version. This PR needs to be merged. Makepad PR #651

the Makepad PR has been merged in now.

@ZhangHanDong
Copy link
Contributor Author

Screenshot 2025-02-24 at 18 30 57

Add a header and other modifications to the popup user list. But it depends on the new changes of the CommandTextInput component. PR 663.

@ZhangHanDong
Copy link
Contributor Author

Currently, getting room members will cache a list locally. Is there any room for improvement in this solution? @kevinaboos

@ZhangHanDong ZhangHanDong added waiting-on-review This issue is waiting to be reviewed and removed waiting-on-author This issue is waiting on the original author for a response labels Feb 24, 2025
Copy link
Member

@kevinaboos kevinaboos left a comment

Choose a reason for hiding this comment

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

Thanks, great start on this complex feature!

First, there are quite a few places where you're using chars/characters, e.g., in find_mention_trigger_position(). We can't use characters, we always must use unicode graphemes instead.

Second, I don't think we need a cache for the list of members in a room -- that's not something that is used frequently, so we shouldn't really cache it. The room user list will be cached internally by the client's EventCache, so we don't really want to cache it separately.
We can just store the latest list of Vec<RoomMember>s in the MentionInputBar's room_members field (and optionally, also in the TimelineUiState struct if needed), we don't need a full cache.

@kevinaboos kevinaboos added waiting-on-author This issue is waiting on the original author for a response and removed waiting-on-review This issue is waiting to be reviewed labels Feb 25, 2025
@kevinaboos
Copy link
Member

I recently added support for an EditingPane that allows the user to edit messages. This EditingPane widget contains its own instance of the TextInput widget, which is completely separate from the message input_bar that you've modified here in this PR.

We need to support mentioning users in both the existing input_bar's TextInput at the bottom of the RoomScreen, and in the new edit_text_input TextInput within the EditingPane.

Can you refactor this PR to provide a MentionableTextInput widget that we can instantiate in multiple places? Then we can try it out in both the RoomScreen's input_bar and in the EditingPane's TextInput.

@ZhangHanDong ZhangHanDong added waiting-on-review This issue is waiting to be reviewed and removed waiting-on-author This issue is waiting on the original author for a response labels Mar 3, 2025
@ZhangHanDong
Copy link
Contributor Author

@kevinaboos ok. I have processed your previous review suggestions, and next I will look at the EditingPane requirement.

@ZhangHanDong ZhangHanDong added waiting-on-author This issue is waiting on the original author for a response and removed waiting-on-review This issue is waiting to be reviewed labels Mar 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-on-author This issue is waiting on the original author for a response
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants