Alkwa/dont render html messages#1149
Conversation
alkwa-msft
requested review from
JamesBurnside,
PorterNan,
alcail,
anjulgarg,
mgamis-msft and
prprabhu-ms
as code owners
| private onChatMessageReceived = (event: ChatMessageReceivedEvent): void => { | ||
| // Today we are avoiding how to render these messages. In the future we can | ||
| // remove this condition and handle this message appropriately. | ||
| if (event.type.startsWith('RichText/')) { |
There was a problem hiding this comment.
Any strong reason for avoiding these?
We can probably convert these to react components and render them.
There was a problem hiding this comment.
There could be any number of control messages. RichText/Media_CallRecording just happens to be one of them. It is better to filter on what we are expecting (Text or RichText/Html)
| private onChatMessageReceived = (event: ChatMessageReceivedEvent): void => { | ||
| // Today we are avoiding how to render these messages. In the future we can | ||
| // remove this condition and handle this message appropriately. | ||
| if (event.type !== 'Text' && event.type !== 'RichText/Html') { |
There was a problem hiding this comment.
Do we not need a condition for 'html'? https://github.com/Azure/communication-ui-library/pull/946/files
Looking to avoid breaking: #824
There was a problem hiding this comment.
nit: convert the type we are checking to lowercase and use the enum ACSKnownMessageType we have for checking against
There was a problem hiding this comment.
Yea, 'html' type is coming from teams sometimes - not sure if that is also the case for notifications
There was a problem hiding this comment.
Huge inconsistence of type name/lower/upper cases from chatThreadClient.getMessages() and these notifications, which should be resolved by our chat sdk in their further release
There was a problem hiding this comment.
Yep I am going to toLowerCase() and do checks against text richtext/html and html
all other messages are currently dropped.
| private onChatMessageReceived = (event: ChatMessageReceivedEvent): void => { | ||
| // Today we are avoiding how to render these messages. In the future we can | ||
| // remove this condition and handle this message appropriately. | ||
| if (event.type !== 'Text' && event.type !== 'RichText/Html') { |
There was a problem hiding this comment.
Are there concerns that we have to do it in stateful client? Since it is our component + selector's responsibility to render the message (and they don't support this type well), messages filter should be happening in messageThreadSelector.tsx - On the other hand, stateful client should always provide accurate data directly coming from js sdk (so developers can read messages and render them themselves even without our implementation)
We already have some filterings there in messageThreadSelector.tsx, I wonder why it is not working, I guess there could be some leakings for our conditions there
There was a problem hiding this comment.
Sync with Alex offline, the big limitation for now is we froze api changes so we are not able to support more types in convertEventType() function above - for short term, this change makes a lot of change before we release the lock of api changes
In long term, we will need to open a bug and add new api changes and move this logic into selector after first GA release
|
According to my previous experience, 2 scenarioes might need to be tested for this PR around html type:
chatted with Ash and we can also potentially get html, so I am account for it as well. |
…tml-messages' into alkwa/dont-render-html-messages
6 participants
What
We want to avoid rendering messages from Teams when a meeting is being recorded
Why
This content is not readable and creates a lot of noise in the chat thread. We will enable it again when we have a good way to display this information.
How Tested
Tested locally in storybook
Process & policy checklist
Is this a breaking change?
This is fixing a bug for now.