-
Notifications
You must be signed in to change notification settings - Fork 240
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: support @mention in mid message #3043
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woohoo. This is shaping up really nicely!!
Gave it a good test and noticed a few things, which I've tried to highlight in this video:
CleanShot.2024-02-07.at.15.04.22.mp4
- Accepting a suggestion should add a space
- Using left and right cursor should hide the suggestion popover
- After accepting a suggestion, hitting backspace and adding a character on the end shouldn't show "No matching files found" (see Slack's implementation for how we should probably handle it)
- Triggered some kind of race condition with editing (perhaps unrelated to this PR)
@toolmantim thanks for testing this out. Just updated the branch with the following changes, all covered by the updated e2e test as well
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super close! It still shows "No matching files found" (and doesn't dismiss the popover when you send the message)
CleanShot.2024-02-07.at.22.54.14.mp4
vs
CleanShot.2024-02-07.at.23.01.03.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Only noticed one small thing:
It seems that the "No matching files found" message doesn't go away when using the arrow keys to go left or right
ChatMiddle@.mov
@toolmantim @umpox Thanks for testing the change so thoroughly! I've manually tested it and updated the tests. Please let me know if it is not working correctly for you. Screen.Recording.2024-02-07.at.12.39.51.PM.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me it still seems to lose the files when you edit the message:
CleanShot.2024-02-08.at.14.23.05.mp4
And the question mark case is showing "No matching files":
CleanShot.2024-02-08.at.14.24.09.mp4
That's unrelated to this PR since this PR is for adding @-file in mid-sentence support. Will work on that in my next PR.
Thanks! I will update and make sure to add tests to cover this tomorrow. Surprise the current one doesn't 🫠 |
Ah, I didn't realise sorry! I didn't check main 👍🏼 |
np! Just updated the PR with tests too. i think the test should cover all the expected behavior now? For edits, it should work the same as press UP for history input right? I can look into that next 👍 |
🎉 |
CLOSE #2633
This PR includes changes to support @mention in mid message:
Test plan