-
Notifications
You must be signed in to change notification settings - Fork 78
[Chat] Announce File Attachments in New Messages #4438
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
Changes from all commits
11b1131
557915e
df3af6d
38b53c6
3491446
6df72fe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| { | ||
| "type": "patch", | ||
| "area": "fix", | ||
| "workstream": "File Sharing", | ||
| "comment": "Add Chat Attachments in Live Messages", | ||
| "packageName": "@azure/communication-react", | ||
| "email": "109105353+jpeng-ms@users.noreply.github.com", | ||
| "dependentChangeType": "patch" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| { | ||
| "type": "patch", | ||
| "area": "fix", | ||
| "workstream": "File Sharing", | ||
| "comment": "Add Chat Attachments in Live Messages", | ||
| "packageName": "@azure/communication-react", | ||
| "email": "109105353+jpeng-ms@users.noreply.github.com", | ||
| "dependentChangeType": "patch" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,9 @@ import LiveMessage from '../Announcer/LiveMessage'; | |
| /* @conditional-compile-remove(mention) */ | ||
| import { defaultOnMentionRender } from './MentionRenderer'; | ||
| import DOMPurify from 'dompurify'; | ||
| import { _AttachmentDownloadCardsStrings } from '../AttachmentDownloadCards'; | ||
| /* @conditional-compile-remove(attachment-download) */ | ||
| import { AttachmentMetadata } from '../../types'; | ||
|
|
||
| type ChatMessageContentProps = { | ||
| message: ChatMessage; | ||
|
|
@@ -164,9 +167,12 @@ export const BlockedMessageContent = (props: BlockedMessageContentProps): JSX.El | |
| }; | ||
|
|
||
| const extractContentForAllyMessage = (props: ChatMessageContentProps): string => { | ||
| if (props.message.content) { | ||
| let attachments = undefined; | ||
| /* @conditional-compile-remove(attachment-download) */ | ||
| attachments = props.message.attachments; | ||
| if (props.message.content || attachments) { | ||
|
jimchou-dev marked this conversation as resolved.
|
||
| // Replace all <img> tags with 'image' for aria. | ||
| const parsedContent = DOMPurify.sanitize(props.message.content, { | ||
| const parsedContent = DOMPurify.sanitize(props.message.content ?? '', { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this change needed?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. because line 170
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like sanitize can take
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it doesn't, it takes |
||
| ALLOWED_TAGS: ['img'], | ||
| RETURN_DOM_FRAGMENT: true | ||
| }); | ||
|
|
@@ -180,6 +186,16 @@ const extractContentForAllyMessage = (props: ChatMessageContentProps): string => | |
| parsedContent.replaceChild(imageTextNode, child); | ||
| }); | ||
|
|
||
| // Inject attachment names for aria. | ||
| if (attachments) { | ||
|
jpeng-ms marked this conversation as resolved.
|
||
| let attachmentList = ''; | ||
| /* @conditional-compile-remove(attachment-download) */ | ||
| attachmentList = attachmentCardGroupDescription(props); | ||
| const attachmentTextNode = document.createElement('div'); | ||
| attachmentTextNode.innerHTML = ` ${attachmentList} `; | ||
| parsedContent.appendChild(attachmentTextNode); | ||
| } | ||
|
jpeng-ms marked this conversation as resolved.
|
||
|
|
||
| // Strip all html tags from the content for aria. | ||
| const message = DOMPurify.sanitize(parsedContent, { ALLOWED_TAGS: [] }); | ||
| return message; | ||
|
|
@@ -207,6 +223,29 @@ const messageContentAriaText = (props: ChatMessageContentProps): string | undefi | |
| }); | ||
| }; | ||
|
|
||
| /* @conditional-compile-remove(attachment-download) */ | ||
| const attachmentCardGroupDescription = (props: ChatMessageContentProps): string => { | ||
| const attachments = props.message.attachments; | ||
| return getAttachmentCountLiveMessage(attachments ?? [], props.strings.attachmentCardGroupMessage); | ||
| }; | ||
|
|
||
| /* @conditional-compile-remove(attachment-download) */ | ||
| /** | ||
| * @private | ||
| */ | ||
| export const getAttachmentCountLiveMessage = ( | ||
| attachments: AttachmentMetadata[], | ||
| attachmentCardGroupMessage: string | ||
| ): string => { | ||
| if (attachments.length === 0) { | ||
| return ''; | ||
| } | ||
| return _formatString(attachmentCardGroupMessage, { | ||
| attachmentCount: `${attachments.length}` | ||
| }); | ||
| return ''; | ||
| }; | ||
|
|
||
| const defaultOnRenderInlineImage = (inlineImage: InlineImage): JSX.Element => { | ||
| return ( | ||
| <img | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
Not necessarily related to this PR but for @JoshuaLai to confirm, do we still need the type of
_AttachmentDownloadCardsStringsand the function ofuseLocaleStringsTrampoline?