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 support for @mentions in the chat content #2668

Merged
merged 24 commits into from
Oct 13, 2023

Conversation

musale
Copy link
Contributor

@musale musale commented Aug 16, 2023

Closes #2189

PR Type

  • Feature

Description of the changes

  • Add support for @mention of users and @mention of other entities.

PR checklist

  • Project builds (yarn build) and changes have been tested in at least two supported browsers (Edge + non-Chromium based browser)
  • All public APIs (classes, methods, etc) have been documented following the jsdoc syntax
  • Stories have been added and existing stories have been tested
  • Added appropriate documentation. Docs PR:
  • License header has been added to all new source files (yarn setLicense)
  • Contains NO breaking changes

Other information

  • The person-card flyout is appearing below the message items. z-index seems not to work as expected.

image

UPDATE

Fixed the flyout showing below the message items issue:

image

musale added 6 commits August 16, 2023 17:20
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>
@musale musale added this to the Chat - Public Preview milestone Aug 16, 2023
@musale musale requested a review from a team as a code owner August 16, 2023 15:19
@microsoft-github-policy-service
Copy link
Contributor

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:

  • I have verified a documentation PR has been linked and is approved (or not applicable)
  • I have ran this PR locally and have tested the fix/feature
  • I have verified that stories have been added to storybook (or not applicable)
  • I have tested existing stories in storybook to verify no regression has occured
  • I have tested the solution in at least two browsers (Edge + 1 non-Chromium based browser)

@musale musale linked an issue Aug 16, 2023 that may be closed by this pull request
musale added 8 commits August 28, 2023 22:06
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>
Signed-off-by: Martin Musale <martinmusale@microsoft.com>
Signed-off-by: Martin Musale <martinmusale@microsoft.com>
Signed-off-by: Martin Musale <martinmusale@microsoft.com>
@musale musale requested a review from gavinbarron September 5, 2023 09:59
@sebastienlevert
Copy link
Contributor

sebastienlevert commented Sep 7, 2023

  • When receiving new messages via WebSockets, we don't provide the hover effect that we do when fetching the messages via the API.
  • Multiple mentions should have more spacing
    image
  • Message send box is now hovering parts of the last message
    image
  • In narrow scenarios, there are stacking context that seems pretty wrong 😂
    image

@musale
Copy link
Contributor Author

musale commented Sep 26, 2023

Hello @sebastienlevert

When receiving new messages via WebSockets, we don't provide the hover effect that we do when fetching the messages via the API.

Do you mean the blue "new message" popup?

Multiple mentions should have more spacing

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.

Message send box is now hovering parts of the last message

Could padding it at the top like teams be a favorable update?

image

In narrow scenarios, there are stacking context that seems pretty wrong

Fixing this up :D

@gavinbarron
Copy link
Member

When receiving new messages via WebSockets, we don't provide the hover effect that we do when fetching the messages via the API.
Do you mean the blue "new message" popup?

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.

Multiple mentions should have more spacing
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.

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 &nbsp;, so if we did this that should help here. This would result in something like <mgt-person ...>Sebastien</mgt-person>&nbsp;<mgt-person ...>Martin>&nbsp;</mgt-person>&nbsp;<mgt-person ...>Musale</mgt-person>&nbsp;

Message send box is now hovering parts of the last message
Could padding it at the top like teams be a favorable update?

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

musale added 4 commits October 2, 2023 12:46
…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>
@github-actions
Copy link

github-actions bot commented Oct 9, 2023

🚀 New react-contoso sample application deployed here

packages/mgt-chat/src/utils/mentions.tsx Outdated Show resolved Hide resolved
packages/mgt-chat/src/utils/mentions.tsx Outdated Show resolved Hide resolved
packages/mgt-chat/src/utils/mentions.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@sebastienlevert sebastienlevert left a 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>
@github-actions
Copy link

🚀 New react-contoso sample application deployed here

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs: Author Feedback Issue needs response from issue author label Oct 12, 2023
@github-actions
Copy link

🚀 New react-contoso sample application deployed here

@github-actions
Copy link

🚀 New react-contoso sample application deployed here

@gavinbarron
Copy link
Member

@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

@github-actions
Copy link

🚀 New react-contoso sample application deployed here

@gavinbarron
Copy link
Member

Changes look good to me:
image

Copy link
Member

@gavinbarron gavinbarron left a comment

Choose a reason for hiding this comment

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

🚀

@gavinbarron gavinbarron merged commit 7ff602b into next/mgt-chat Oct 13, 2023
8 checks passed
@gavinbarron gavinbarron deleted the at-mentions branch October 13, 2023 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build: React Contoso Needs: Author Feedback Issue needs response from issue author
Projects
Archived in project
3 participants