-
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 @mention support in send box #2873
Conversation
🚀 New react-contoso sample application deployed here |
1 similar comment
🚀 New react-contoso sample application deployed here |
🚀 New react-contoso sample application deployed here |
Can we get a Can we also remove users from the mention selector when they were mentioned? When we send a message, temporarily, we see the 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. |
@sebastienlevert I've done some updates and here are responses for the questions above:
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:
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.
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.
This one is weird. I reproduced with:
However when I click on search it comes back to normal. I also tried:
I'm not sure the root of this so let me also dig into it ASAP. |
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. |
🚀 New react-contoso sample application deployed here |
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>
9ae160f
to
6360fd9
Compare
🚀 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.
LGTM
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.
I still have the msft-mention renders temporairly
This won't be fixed here, as explained offline. |
8c33cc5
🚀 New react-contoso sample application deployed here |
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 |
Closing to test if statuses will trigger on re-opening |
🚀 New react-contoso sample application deployed here |
This reverts commit 8c33cc5.
🚀 New react-contoso sample application deployed here |
🚀 New react-contoso sample application deployed here |
@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? |
This reverts commit d7a2e51.
🚀 New react-contoso sample application deployed here |
|
Closes #2828
PR Type
Description of the changes
Added support to show a list of people/users when you type
@
in send box and some textPR checklist
yarn build
) and changes have been tested in at least two supported browsers (Edge + non-Chromium based browser)yarn setLicense
)Other information