twilio-video-app-react icon indicating copy to clipboard operation
twilio-video-app-react copied to clipboard

Grid mode use sdk v3

Open timmydoza opened this issue 2 years ago • 2 comments

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.

  • [ ] I acknowledge that all my contributions will be made under the project's license.

Pull Request Details

JIRA link(s):

Description

A description of what this PR does.

Burndown

Before review

  • [ ] Updated CHANGELOG.md if necessary
  • [ ] Added unit tests if necessary
  • [ ] Updated affected documentation
  • [ ] Verified locally with npm test
  • [ ] Manually sanity tested running locally
  • [ ] Included screenshot as PR comment (if needed)
  • [ ] Ready for review

Before merge

  • [ ] Got one or more +1s
  • [ ] Re-tested if necessary

timmydoza avatar May 11 '22 22:05 timmydoza

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 avatar May 12 '22 15:05 timmydoza

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.

manjeshbhargav avatar May 12 '22 16:05 manjeshbhargav