Skip to content

Conversation

@gabriel-jt
Copy link
Contributor

@gabriel-jt gabriel-jt commented Jul 22, 2021

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

Added virtual background feature

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

@gabriel-jt gabriel-jt requested a review from charliesantos July 22, 2021 22:52
@gabriel-jt gabriel-jt changed the base branch from master to feature/virtualbackground July 22, 2021 22:52
Copy link
Collaborator

@charliesantos charliesantos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work Gabe! I like how simple it looks :)
Just a couple of comments. Also, please make sure CI passes.

"ts-node": "^9.1.1",
"twilio": "3.63.1",
"twilio-video": "^2.14.0",
"twilio-video": "^2.15.2",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also please update @twilio/video-processors to latest?

backgroundSettings = renderResult.current[0];
expect(backgroundSettings.type).toEqual('image');
expect(backgroundSettings.index).toEqual(2);
expect(mockVideoTrack.addProcessor).toHaveBeenCalled();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have a test that checks addProcessor was called with the virtualBackground param? I think we need to update line 75 as well. Check addProcessor was called with the blur processor param.

@gabriel-jt gabriel-jt merged commit 171b376 into feature/virtualbackground Jul 26, 2021
charliesantos pushed a commit that referenced this pull request Aug 9, 2021
* Moved more menu to the center of the menu bar

* VIDEO-5731 | Added background option to More Menu, selection dialog, and unit tests (#6)

Added background option to More Menu, selection dialog, and unit tests
Co-authored-by: Gabe Espinosa <[email protected]>

* VIDEO-5733 | Added npm script to copy video processor sdk assets to public folder (#8)

* Added npm script to copy video processor sdk assets to public folder

* added postinstall npm script

Co-authored-by: Gabe Espinosa <[email protected]>

* added rimraf and copyfiles dependency to package.json

* VIDEO-5732 | Added background selection UI (#7)

Added background selection UI

Co-authored-by: Gabe Espinosa <[email protected]>
Co-authored-by: timmydoza <[email protected]>

* VIDEO-5734/blur background feature (#550)

Implemented blur background feature (useBackgroundSettings hook and tests)
Co-authored-by: Gabe Espinosa <[email protected]>

* VIDEO-5735/Add virtual background feature (#557)

Added virtual background feature and unit tests
Co-authored-by: Gabe Espinosa <[email protected]>

* VIDEO-5735 | Add isSupported check and unit test (#560)

add isSupported check and unit test
Co-authored-by: Gabe Espinosa <[email protected]>

* VIDEO-6368 | wait to load image before removing processor (#570)

* wait to load image before removing processor
Co-authored-by: Gabe Espinosa <[email protected]>

* updated twilio-video sdk

* fixed breaking tests

* update package.json version and changelog

* update change log, package.json, and revisions

* Update CHANGELOG.md

Co-authored-by: Gabe Espinosa <[email protected]>
Co-authored-by: timmydoza <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants