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

Closed
wants to merge 26 commits into from
Closed

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.

Signed-off-by: Martin Musale <martinmusale@microsoft.com>
Signed-off-by: Martin Musale <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: Martin Musale <martinmusale@microsoft.com>
Signed-off-by: Martin Musale <martinmusale@microsoft.com>
Signed-off-by: Martin Musale <martinmusale@microsoft.com>
@musale musale force-pushed the mention-in-sendbox branch from 9ae160f to 6360fd9 Compare February 12, 2024 10:04
Copy link

🚀 New react-contoso sample application deployed here

gavinbarron
gavinbarron previously approved these changes Mar 6, 2024
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

@musale
Copy link
Contributor Author

musale commented Jul 18, 2024

I still have the msft-mention renders temporairly

This won't be fixed here, as explained offline.

@sebastienlevert sebastienlevert self-requested a review August 28, 2024 15:26
@sebastienlevert sebastienlevert enabled auto-merge (squash) August 28, 2024 15:27
Mnickii
Mnickii previously approved these changes Aug 29, 2024
@sebastienlevert
Copy link
Contributor

@Mnickii @musale can you check why GitOps and license can't be validates to auto-merge?

@Mnickii Mnickii dismissed stale reviews from sebastienlevert, gavinbarron, and themself via 8c33cc5 August 30, 2024 12:11
Copy link

🚀 New react-contoso sample application deployed here

@Mnickii
Copy link
Collaborator

Mnickii commented Aug 30, 2024

The statuses seem to be stuck, trying the solutions detailed here https://stackoverflow.com/questions/52200096/github-pull-request-waiting-for-status-to-be-reported to trigger them

@Mnickii
Copy link
Collaborator

Mnickii commented Aug 30, 2024

Closing to test if statuses will trigger on re-opening

@Mnickii Mnickii closed this Aug 30, 2024
auto-merge was automatically disabled August 30, 2024 12:22

Pull request was closed

@Mnickii Mnickii reopened this Aug 30, 2024
Copy link

🚀 New react-contoso sample application deployed here

Copy link

🚀 New react-contoso sample application deployed here

Copy link

github-actions bot commented Sep 2, 2024

🚀 New react-contoso sample application deployed here

@Mnickii
Copy link
Collaborator

Mnickii commented Sep 2, 2024

@musale, it seems you'd have to open a new PR for these statuses to pass. Disabling and re-enabling the status checks, and triggering new checks doesn't seem to work here, unless we've got other ideas to try?

Copy link

github-actions bot commented Sep 2, 2024

🚀 New react-contoso sample application deployed here

Copy link

github-actions bot commented Sep 2, 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

@musale
Copy link
Contributor Author

musale commented Sep 2, 2024

Closing in favour of #3281 cc @Mnickii

@musale musale closed this Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

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