Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implementation of CardPart Video View #265

Merged
merged 8 commits into from Oct 28, 2020
Merged

Conversation

badrinathvm
Copy link
Contributor

@badrinathvm badrinathvm commented Oct 27, 2020

Before you make a Pull Request, read the important guidelines:

Implementation of CardPartVideoView.

Video_iOS

Issue Link 馃敆

  • New Feature
  • Does it break any existing functionality? - None

Goals of this PR 馃帀

  • Why is the change important? - Addition of new component to CardParts for displaying Videos
  • What does this fix?
  • How far has it been tested? - Unit / UI Tested

How Has This Been Tested 馃攳

Please let us know if you have tested your PR and if we need to reproduce the issues. Also, please let us know if we need any relevant information for running the tests.

  • User Interface Testing : YES
  • Application Testing: YES

Test Configuration 馃懢

  • Xcode version: 12.0
  • Device/Simulator: 11.0
  • iOS version || MacOSX version: Catalina - 10.15.7

Things to check on 馃幆

  • My Pull Request code follows the coding standards and styles of the project
  • I have worked on unit tests and reviewed my code to the best of my ability
  • I have used comments to make other coders understand my code better
  • My changes are good to go without any warnings
  • I have added unit tests both for the happy and sad path
  • All of my unit tests pass successfully before pushing the PR
  • I have made sure all dependent downstream changes impacted by my PR are working

@rcole34
Copy link
Contributor

rcole34 commented Oct 27, 2020

@croossin @bharathmurs this seems to be working fine for Badari, but I wasn't sure if this is the "CardParts" way of adding a subview from another controller? I was thinking he might need to set the viewController property of the CardPartsVideoView to get this snippet from setupCardParts to run, but it seems to work without that also https://github.com/intuit/CardParts/blob/master/CardParts/src/Classes/CardPartsViewController.swift#L85:L88

@badrinathvm
Copy link
Contributor Author

badrinathvm commented Oct 27, 2020

@rcole34 self.viewContoller.addChild(.. c2c0e5c from CardPartView is being removed in CardPartVideoView, the above comment might is not valid anymore.

Copy link
Contributor

@croossin croossin left a comment

Choose a reason for hiding this comment

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

Looks good to me.

CardParts/src/Classes/Card Parts/CardPartVideoView.swift Outdated Show resolved Hide resolved
@croossin
Copy link
Contributor

Just curious does this support picture-in-picture out of the box

@croossin croossin merged commit 5c3c346 into master Oct 28, 2020
@croossin croossin deleted the task/cardpartvideocontroller branch October 28, 2020 00:59
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.

None yet

3 participants