-
Notifications
You must be signed in to change notification settings - Fork 310
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 support for @mentions
in the chat content
#2668
Conversation
Signed-off-by: Musale Martin <martinmusale@microsoft.com>
Signed-off-by: Musale Martin <martinmusale@microsoft.com>
Signed-off-by: Musale Martin <martinmusale@microsoft.com>
Signed-off-by: Musale Martin <martinmusale@microsoft.com>
Signed-off-by: Musale Martin <martinmusale@microsoft.com>
Signed-off-by: Musale Martin <martinmusale@microsoft.com>
Thank you for creating a Pull Request @musale. This is a checklist for the PR reviewer(s) to complete before approving and merging this PR:
|
packages/mgt-chat/src/statefulClient/StatefulGraphChatClient.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: Martin Musale <martinmusale@microsoft.com>
Signed-off-by: Martin Musale <martinmusale@microsoft.com>
Signed-off-by: Martin Musale <martinmusale@microsoft.com>
…raph-toolkit into at-mentions
Signed-off-by: Martin Musale <martinmusale@microsoft.com>
Signed-off-by: Martin Musale <martinmusale@microsoft.com>
Signed-off-by: Martin Musale <martinmusale@microsoft.com>
Signed-off-by: Martin Musale <martinmusale@microsoft.com>
Hello @sebastienlevert
Do you mean the blue "new message" popup?
This one is a little bit tricky. ACS are processing the mentions as single tokens. So "Adele Vance" will be two independent objects. I could match the userIds and try to "put them together" then space appropriately but I can only access one "token" at a time.
Could padding it at the top like teams be a favorable update?
Fixing this up :D |
No, if you have a chat open in the test application and then use Teams to send a message on the same thread that message is received by the GraphNotificiationClient via Web Sockets. It appears that the transformation is not being applied to the message when it is received via this channel.
I don't think we need to be combining each name in a mention together let's try and avoid complexity like that when we can. Looking at Teams in the web they ensure that every name in a mention is followed by an
No, there's a stacking problem now, i.e. the send message box is floating over the chat thread area. We need to ensure that the two elements do not overlap and layout correctly |
…raph-toolkit into at-mentions Signed-off-by: Musale Martin <martinmusale@microsoft.com>
Signed-off-by: Musale Martin <martinmusale@microsoft.com>
Signed-off-by: Musale Martin <martinmusale@microsoft.com>
🚀 New react-contoso sample application deployed here |
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.
This works great in narrow and larger screens, also in both reading from the API and receiving from the socket! Great job @musale!
Remove unnecessary assertions Signed-off-by: Martin Musale <martinmusale@microsoft.com>
🚀 New react-contoso sample application deployed here |
🚀 New react-contoso sample application deployed here |
🚀 New react-contoso sample application deployed here |
@musale + @sebastienlevert I think I got the merge here correct, but this does need another functional test before it's ready to merge. Hopefully now we see mentions correctly displayed while messages with unsupported content rendering the link out to the Team web client |
🚀 New react-contoso sample application deployed here |
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.
🚀
Closes #2189
PR Type
Description of the changes
@mention
of users and@mention
of other entities.PR checklist
yarn build
) and changes have been tested in at least two supported browsers (Edge + non-Chromium based browser)yarn setLicense
)Other information
z-index
seems not to work as expected.UPDATE
Fixed the flyout showing below the message items issue: