Person Icons, Emojis, Images, Group Title +N #3
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
plasne
reviewed
Jan 5, 2024
| } | ||
|
|
||
| return removeHtmlPTags(previewString); | ||
| // removes <p> tags from the string | ||
| previewString = removeHtmlPTags(previewString); |
There was a problem hiding this comment.
This doesn't address <span /> or <div /> tags or any number of other HTML that could appear. Maybe consider: https://www.npmjs.com/package/html-to-text. You may still want to keep the "Sent an image." path and the emoji path.
plasne
requested changes
Jan 5, 2024
| // handle images | ||
| const graphImageUrlRegex = /<img[^>]+?src=["']https:\/\/graph\.microsoft\.com\/[^"'>]+?["']/; | ||
|
|
||
| const imageMatch = content.match(graphImageUrlRegex); |
There was a problem hiding this comment.
This crashes with an error if content is null | undefined. You can probably just shortcut out of this method if there is no preview.
| // define the JSX for FluentUI Icons + Styling | ||
| const oneOnOneProfilePicture = <ChatListItemIcon chatType="oneOnOne" />; | ||
| const GroupProfilePicture = <ChatListItemIcon chatType="group" />; | ||
|
|
||
| const other = chatObj.members?.find(m => (m as AadUserConversationMember).userId !== myId); | ||
| const otherAad = other as AadUserConversationMember; |
There was a problem hiding this comment.
We probably want to have a bot icon for bots which are applications, not conversation members. Or at a minimum they should use the default icon and not the user's icon.
Because the chat component comes from ACS, it doesn't actually send info about the bot, so that control just uses the default icon.
| } | ||
|
|
||
| return removeHtmlPTags(previewString); | ||
| // removes <p> tags from the string | ||
| previewString = removeHtmlPTags(previewString); |
There was a problem hiding this comment.
It seems like this should happen on content not necessarily previewString and be moved up above the "handle general chats" section.
| } else if (previewMessage?.from?.application?.displayName) { | ||
| previewString = previewMessage?.from?.application?.displayName + ': ' + previewMessage?.body?.content; | ||
| previewString = previewMessage?.from?.application?.displayName + ': ' + content; | ||
| } | ||
|
|
||
| // handle all events | ||
| if (previewMessage?.eventDetail) { |
There was a problem hiding this comment.
Line 234 handles the preview message being from a bot, Line 238 handles events like "Member Added"
plasne
reviewed
Jan 5, 2024
|
Your demo was great. I think there was one issue with a drag-and-drop image. If you fix that, then I will approve. Thanks, Peter. |
TechnicallyWilliams
approved these changes
Jan 9, 2024
…raph-toolkit into andrewDoing/person-icon
…raph-toolkit into andrewDoing/person-icon
|
plasne
approved these changes
Jan 9, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes 307, 360, 370
PR Type
Description of the changes
PR checklist
yarn build) and changes have been tested in at least two supported browsers (Edge + non-Chromium based browser)yarn setLicense)Other information