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

Added change of camera stream, update README #5

Merged
merged 2 commits into from Jul 2, 2018

Conversation

Projects
None yet
2 participants
@Giorat
Copy link
Contributor

Giorat commented Jun 8, 2018

I added possibility to change camera stream.

Update README too with logo and badges! 馃槃

I have also added yarn lock and a module logo 馃憤

@Giorat

This comment has been minimized.

Copy link
Contributor Author

Giorat commented Jun 8, 2018

You can also set this description for the repository " 馃帴 Webcam component for VUEJS 馃摲 "
image

@Giorat

This comment has been minimized.

Copy link
Contributor Author

Giorat commented Jun 11, 2018

@VinceG did you check my changes?

@VinceG

This comment has been minimized.

Copy link
Owner

VinceG commented Jun 15, 2018

@Giorat Thank You. i'll give this a try and merge if everything goes well. I appreciate the help with this.

@Giorat

This comment has been minimized.

Copy link
Contributor Author

Giorat commented Jun 21, 2018

Any news on this?

@Giorat

This comment has been minimized.

Copy link
Contributor Author

Giorat commented Jun 29, 2018

Still nothing? Sorry for the comments but I really want to improve and help you out! ;) This is also my first pull request

@VinceG VinceG merged commit 6dd9bf4 into VinceG:master Jul 2, 2018

@VinceG

This comment has been minimized.

Copy link
Owner

VinceG commented Jul 2, 2018

@Giorat This PR was a hard one to merge as is. There were a few issues with the PR that i had to update before merging:

  1. All the events emitted were removed, i am not sure why.
  2. There were instances of console.log and alerts that i had to remove. console logs and alerts do not help when it comes to third party libraries. the end user can't control those alerts.
  3. The idea of changing the webcam by incrementing the index is strange. If a user has 100 webcams and he want to use webcam no. 25 he has to call the change webcam 24 times? I changed it to allow the option of specifying the webcam to use as well as getting a list of available webcams. By default it loads none of them.
  4. Minor other changes to make it more configurable.

I also updated the demo to showcase the how to switch the webcams and capture, in addition to the other events emitted.

screen shot 2018-07-01 at 10 26 33 pm

@Giorat

This comment has been minimized.

Copy link
Contributor Author

Giorat commented Jul 2, 2018

I considered that users usually have just 2 cameras on phones, but you're right a real business might have hundreds of connected cameras. Sorry for my mistakes, this was my first real pull request.

I'll try to improve with your tips.

Thanks, Riccardo.

@VinceG

This comment has been minimized.

Copy link
Owner

VinceG commented Jul 2, 2018

@Giorat you did great. Thank you for the contribution and the help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.