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 @mention support in send box #2873

Open
wants to merge 22 commits into
base: next/mgt-chat
Choose a base branch
from

Conversation

musale
Copy link
Contributor

@musale musale commented Nov 23, 2023

Closes #2828

PR Type

  • Feature

Description of the changes

Added support to show a list of people/users when you type @ in send box and some text

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

Copy link

🚀 New react-contoso sample application deployed here

1 similar comment
Copy link

🚀 New react-contoso sample application deployed here

@musale musale marked this pull request as ready for review November 24, 2023 11:31
@musale musale requested a review from a team as a code owner November 24, 2023 11:31
@musale musale added this to the Chat - GA milestone Nov 24, 2023
Copy link

🚀 New react-contoso sample application deployed here

@sebastienlevert
Copy link
Contributor

sebastienlevert commented Nov 24, 2023

Can we get a mgt-person instead of the native ACS controls?

image

Can we also remove users from the mention selector when they were mentioned?

image

When we send a message, temporarily, we see the msft-mention in the message component. (for maybe 500ms).

When loading the chat component, it now injects some styling that breaks some of styles of the react contoso app. When navigating elsewhere, it REALLY breaks the app.

image

@musale
Copy link
Contributor Author

musale commented Nov 28, 2023

@sebastienlevert I've done some updates and here are responses for the questions above:

Can we get a mgt-person instead of the native ACS controls?

Yes. However, that is overriding the rendering of the suggestions and as it is, it's too buggy especially for keyboard navigation. I have raised a couple of issues I hope will be addressed soon. The PRs are:

Can we also remove users from the mention selector when they were mentioned?

Yes and No. Yes, using the selected suggestion from the mention look up props, we can do some filters and get what we want. No because that is only applicable in the part of the mentions API that allows you to customize the suggestions. That part is the one I've talked about above that is buggy. Teams supports what we already have, and I think that should be good enough.

When we send a message, temporarily, we see the msft-mention in the message component. (for maybe 500ms).

This one is a bit tough. I'll drill down a bit to figure out where the lag comes from. We're doing some pretty much more processing when we encounter a mention so that's the first place to check.

When loading the chat component, it now injects some styling that breaks some of styles of the react contoso app. When navigating elsewhere, it REALLY breaks the app.

This one is weird. I reproduced with:

  • Load app
  • Click on chat
  • Click on Taxonomy.

However when I click on search it comes back to normal.

I also tried:

  • load the app
  • click on taxonomy
  • click on chat. Nothing happens.
  • Click on taxonomy. Styling breaks.

I'm not sure the root of this so let me also dig into it ASAP.
UPDATE: happens in next/mgt-chat too. Raising it as a separate issue.

@sebastienlevert
Copy link
Contributor

Should we hold until ACS has fixes for these issues?

@musale
Copy link
Contributor Author

musale commented Nov 29, 2023

Should we hold until ACS has fixes for these issues?

Yes. I think keyboard navigation is very intuitive for this feature. Almost 90% of it is done, but this would be really nice to have out of the box.

Copy link

github-actions bot commented Dec 4, 2023

🚀 New react-contoso sample application deployed here

@sebastienlevert
Copy link
Contributor

Is it possible we're back at "WIP" state with the current code? I see we lost a couple of features when introducing the MGT Person component (when mentioning somebody the user doesn't get removed, the component is still pretty rough, etc.).

@musale
Copy link
Contributor Author

musale commented Dec 6, 2023

Is it possible we're back at "WIP" state with the current code? I see we lost a couple of features when introducing the MGT Person component (when mentioning somebody the user doesn't get removed, the component is still pretty rough, etc.).

Yes. Customizing the experience is very limited and I still see a couple of new issues which ACS might need to help to fix. For example, keyboard up/down is impossible to implement. I will wait for the update to the initial issue I raised and see what can be done.

Copy link

🚀 New react-contoso sample application deployed here

Copy link

🚀 New react-contoso sample application deployed here

@gavinbarron
Copy link
Member

We need to have the ACS team expose the active parameter for onRenderSuggestionItem in the typings

Added further commentary to the existing issue on this front here Azure/communication-ui-library#3836 (comment)

Copy link

github-actions bot commented Mar 5, 2024

🚀 New react-contoso sample application deployed here

1 similar comment
Copy link

github-actions bot commented Mar 5, 2024

🚀 New react-contoso sample application deployed here

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.

  • In the flyout, the scroll bar is always visible even though it doesn't scroll
    image

  • I feel a pointer cursor should be used to select a user
    image

  • For a very short amount of time, the rendering of the mentioned user shows up as markup
    image

packages/mgt-chat/src/components/Chat/Chat.tsx Outdated Show resolved Hide resolved
packages/mgt-chat/src/components/Chat/Chat.tsx Outdated Show resolved Hide resolved
packages/mgt-chat/src/components/Chat/Chat.tsx Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Mar 6, 2024

Code Coverage

Package Line Rate Branch Rate Complexity Health
mgt-chat.src.statefulClient 100% 100% 0
mgt-chat.src.utils 100% 90% 0
mgt-components.dist.es6.components.mgt-agenda.src.components.mgt-agenda 14% 100% 0
mgt-components.dist.es6.components.mgt-contact.src.components.mgt-contact 62% 100% 0
mgt-components.dist.es6.components.mgt-file-list.mgt-file-upload.src.components.mgt-file-list.mgt-file-upload 43% 100% 0
mgt-components.dist.es6.components.mgt-file-list.src.components.mgt-file-list 62% 100% 0
mgt-components.dist.es6.components.mgt-file.src.components.mgt-file 61% 100% 0
mgt-components.dist.es6.components.mgt-get.src.components.mgt-get 19% 100% 0
mgt-components.dist.es6.components.mgt-login.src.components.mgt-login 64% 100% 0
mgt-components.dist.es6.components.mgt-messages.src.components.mgt-messages 66% 100% 0
mgt-components.dist.es6.components.mgt-organization.src.components.mgt-organization 46% 100% 0
mgt-components.dist.es6.components.mgt-people-picker.src.components.mgt-people-picker 56% 100% 0
mgt-components.dist.es6.components.mgt-people.src.components.mgt-people 72% 100% 0
mgt-components.dist.es6.components.mgt-person-card.src.components.mgt-person-card 59% 33% 0
mgt-components.dist.es6.components.mgt-person.src.components.mgt-person 53% 100% 0
mgt-components.dist.es6.components.mgt-picker.src.components.mgt-picker 78% 100% 0
mgt-components.dist.es6.components.mgt-planner.src.components.mgt-planner 55% 100% 0
mgt-components.dist.es6.components.mgt-profile.src.components.mgt-profile 39% 100% 0
mgt-components.dist.es6.components.mgt-search-box.src.components.mgt-search-box 83% 100% 0
mgt-components.dist.es6.components.mgt-search-results.src.components.mgt-search-results 56% 100% 0
mgt-components.dist.es6.components.mgt-tasks-base.src.components.mgt-tasks-base 86% 100% 0
mgt-components.dist.es6.components.mgt-taxonomy-picker.src.components.mgt-taxonomy-picker 74% 100% 0
mgt-components.dist.es6.components.mgt-teams-channel-picker.src.components.mgt-teams-channel-picker 62% 100% 0
mgt-components.dist.es6.components.mgt-theme-toggle.src.components.mgt-theme-toggle 74% 100% 0
mgt-components.dist.es6.components.mgt-todo.src.components.mgt-todo 78% 100% 0
mgt-components.dist.es6.components.src.components 86% 100% 0
mgt-components.dist.es6.components.sub-components.mgt-arrow-options.src.components.sub-components.mgt-arrow-options 76% 100% 0
mgt-components.dist.es6.components.sub-components.mgt-dot-options.src.components.sub-components.mgt-dot-options 29% 100% 0
mgt-components.dist.es6.components.sub-components.mgt-flyout.src.components.sub-components.mgt-flyout 40% 100% 0
mgt-components.dist.es6.components.sub-components.mgt-spinner.src.components.sub-components.mgt-spinner 92% 100% 0
mgt-components.dist.es6.graph.src.graph 36% 100% 0
mgt-components.dist.es6.src 100% 100% 0
mgt-components.dist.es6.styles.src.styles 73% 100% 0
mgt-components.dist.es6.utils.src.utils 46% 100% 0
mgt-components.src.components 84% 75% 0
mgt-components.src.components.mgt-contact 68% 83% 0
mgt-components.src.components.mgt-file 62% 100% 0
mgt-components.src.components.mgt-file-list 46% 100% 0
mgt-components.src.components.mgt-file-list.mgt-file-upload 49% 86% 0
mgt-components.src.components.mgt-get 22% 100% 0
mgt-components.src.components.mgt-messages 68% 100% 0
mgt-components.src.components.mgt-organization 47% 100% 0
mgt-components.src.components.mgt-person 84% 76% 0
mgt-components.src.components.mgt-person-card 67% 48% 0
mgt-components.src.components.mgt-picker 80% 100% 0
mgt-components.src.components.mgt-profile 40% 100% 0
mgt-components.src.components.mgt-tasks-base 87% 100% 0
mgt-components.src.components.mgt-theme-toggle 100% 100% 0
mgt-components.src.components.mgt-todo 79% 100% 0
mgt-components.src.components.sub-components.mgt-flyout 72% 53% 0
mgt-components.src.components.sub-components.mgt-spinner 100% 100% 0
mgt-components.src.graph 40% 73% 0
mgt-components.src.styles 92% 80% 0
mgt-components.src.utils 82% 46% 0
mgt-element.dist.es6.components.src.components 72% 74% 0
mgt-element.dist.es6.mock.src.mock 91% 77% 0
mgt-element.dist.es6.providers.src.providers 87% 83% 0
mgt-element.dist.es6.src 91% 80% 0
mgt-element.dist.es6.utils.src.utils 67% 73% 0
mgt-element.src 93% 40% 0
mgt-element.src.components 78% 100% 0
mgt-element.src.mock 81% 56% 0
mgt-element.src.providers 83% 91% 0
mgt-element.src.utils 71% 90% 0
Summary 60% (27986 / 46509) 71% (556 / 788) 0

Copy link

github-actions bot commented Mar 6, 2024

🚀 New react-contoso sample application deployed here

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.

LGTM

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.

I still have the msft-mention renders temporairly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress 🚧
Development

Successfully merging this pull request may close these issues.

[mgt-chat] Support for the mention capabilities directly in the message box
4 participants