Composite option to hide local screen share stream #5699
Conversation
carocao-msft
requested review from
JamesBurnside,
JoshuaLai,
Leah-Xia-Microsoft,
alkwa-msft,
dmceachernmsft,
edwardlee-msft,
emlynmac,
jimchou-dev,
jpeng-ms,
mgamis-msft,
palatter,
prabhjot-msft,
vhuseinova-msft and
waynemo
|
Storybook 8 URL https://60c7ae6891f0e90039d7cd54-mghtyblkwc.chromatic.com/ |
@azure/communication-react jest test coverage for stable.
|
@azure/communication-react jest test coverage for beta.
|
| joinCallOptions?: { | ||
| microphoneCheck?: 'requireMicrophoneAvailable' | 'skip'; | ||
| }; | ||
| hideLocalScreenShareStream?: boolean; |
There was a problem hiding this comment.
can you move this inside the existing galleryOptions?
There was a problem hiding this comment.
Avoid booleans, change to something like localScreenshareView: { 'stream' | 'placeholderMessage' } (open to better naming!)
There was a problem hiding this comment.
I like the movement of this as well to gallery options
There was a problem hiding this comment.
Customers understand what the default behaviour is and understand they want to be a little special so they are willing to do this additional change if they want it. It's ok to not have this API on the top level. This is a good discussion as we should be thinking where should these APIs exist.
| if (!localParticipant || !localParticipant.isScreenSharingOn) { | ||
| return null; | ||
| } | ||
| const localScreenSharingNotification = ( |
There was a problem hiding this comment.
We use notification to means that ufd alert UI, is the same UI or the old UI were the tile shows placeholder text and icon?
If its the same we should be using the notification component, if its the old UI change the naming here
There was a problem hiding this comment.
This is the old UI. I just brought back the code changes removed before when changing to show local video stream. What should the name be?
There was a problem hiding this comment.
Your choice, but notification is now a different component we ship so we should avoid that, one option would be localScreenSharingPlaceholder
dmceachernmsft
left a comment
dmceachernmsft
left a comment
There was a problem hiding this comment.
We should also write some e2e tests that meet this scenario as well.
| joinCallOptions?: { | ||
| microphoneCheck?: 'requireMicrophoneAvailable' | 'skip'; | ||
| }; | ||
| hideLocalScreenShareStream?: boolean; |
There was a problem hiding this comment.
I like the movement of this as well to gallery options
| onCreateLocalStreamView, | ||
| onDisposeLocalScreenShareStreamView | ||
| onDisposeLocalScreenShareStreamView, | ||
| hideLocalScreenShareStream |
There was a problem hiding this comment.
if we are specifying there is a default in the JSdocs we should make sure we are setting that here in the code as well instead of relying on undefined.
|
Storybook 8 URL https://60c7ae6891f0e90039d7cd54-vjisedxdyn.chromatic.com/ |
Calling bundle size is not changed.
|
CallWithChat bundle size is not changed.
|
Chat bundle size is not changed.
|
|
Storybook 8 URL https://60c7ae6891f0e90039d7cd54-oxwykdzvpm.chromatic.com/ |
5 participants
What
Composite option to hide local screen share stream
Why
How Tested
Process & policy checklist
Is this a breaking change?