twilio / twilio-video-app-react

A collaboration application built with the twilio-video.js SDK and React.js
Apache License 2.0
1.81k stars 728 forks source link

Grid mode use sdk v3 #677

Open timmydoza opened 2 years ago

timmydoza commented 2 years ago

Contributing to Twilio

All third party contributors acknowledge that any contributions they provide will be made under the same open source license that the open source project is provided under.

Pull Request Details

JIRA link(s):

Description

A description of what this PR does.

Burndown

Before review

Before merge

timmydoza commented 2 years ago

Thanks @manjeshbhargav!

I've updated the branch to use the correct string for 'disabled-by-publisher'. I wasn't able to use the enum from Track.SwitchOffReason because it isn't accessible. I think we need to export the enum on line 8 here. You can see the effect of exporting the enum in this TypeScript Playground example.

Also, for this:

we also need to handle audio tracks being switched off by disabling the audio level indicator and changing the mic icon

I think this will already be handled by the changes to the useIsEnabled hook, as this is what determines when we show the muted icon. Unless, do you mean that we should show the muted icon for switchoff reasons other than 'disabled-by-publisher'?

manjeshbhargav commented 2 years ago

Thanks @manjeshbhargav!

I've updated the branch to use the correct string for 'disabled-by-publisher'. I wasn't able to use the enum from Track.SwitchOffReason because it isn't accessible. I think we need to export the enum on line 8 here. You can see the effect of exporting the enum in this TypeScript Playground example.

Also, for this:

we also need to handle audio tracks being switched off by disabling the audio level indicator and changing the mic icon

I think this will already be handled by the changes to the useIsEnabled hook, as this is what determines when we show the muted icon. Unless, do you mean that we should show the muted icon for switchoff reasons other than 'disabled-by-publisher'?

@timmydoza We need to handle other switch off reasons as well. Either we can re-use the mic disabled icon, or we can use another icon to help us differentiate between disabled and switched off.