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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Daily video calls #542

Merged
merged 43 commits into from Oct 7, 2021
Merged

Conversation

lunchpaillola
Copy link
Contributor

@lunchpaillola lunchpaillola commented Sep 1, 2021

Adding Daily Video Calls to Calendso. Here's a demo of what creating and joining a video room looks like

`https://www.loom.com/share/f1fe710a081a46078621749d532a6e1c```

@vercel
Copy link

vercel bot commented Sep 1, 2021

Someone is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

@vercel
Copy link

vercel bot commented Sep 1, 2021

Someone is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

@amirhmoradi
Copy link

Hi @lunchpaillola , thanks for the great work. I have some feedback if you don't mind :)

I believe we could use already available "isdedicated" concept and avoid hard coding an external provider (ie Daily here) in the event manager (and other places).

For the db side, I would also suggest having a separate table to hold Daily's info and reference the booking id. Core tables shall theoretically be clean of external providers, to allow for better future evolutions.

About migrations, it would be best to have just one migration per PR. You could prepare it by going from a base DB (from latest main branch) and the applying your schema and then generating the migration)

For the emails templates, we could condition the meeting username and password existence and so avoid the hard coded provider condition.

Also, I would suggest having another PR for code indentation fixes and lib/deps updates.

I would love to see your PR merged soon :)

@baileypumfleet baileypumfleet added the ❗️ changes requested Waiting for the author to implement fixes/suggestions label Sep 1, 2021
@lunchpaillola
Copy link
Contributor Author

Hi @lunchpaillola , thanks for the great work. I have some feedback if you don't mind :)

I believe we could use already available "isdedicated" concept and avoid hard coding an external provider (ie Daily here) in the event manager (and other places).

For the db side, I would also suggest having a separate table to hold Daily's info and reference the booking id. Core tables shall theoretically be clean of external providers, to allow for better future evolutions.

About migrations, it would be best to have just one migration per PR. You could prepare it by going from a base DB (from latest main branch) and the applying your schema and then generating the migration)

For the emails templates, we could condition the meeting username and password existence and so avoid the hard coded provider condition.

Also, I would suggest having another PR for code indentation fixes and lib/deps updates.

I would love to see your PR merged soon :)

Thanks for your review and the comments!

Bumps [tailwindcss](https://github.com/tailwindlabs/tailwindcss) from 2.2.14 to 2.2.15.
- [Release notes](https://github.com/tailwindlabs/tailwindcss/releases)
- [Changelog](https://github.com/tailwindlabs/tailwindcss/blob/master/CHANGELOG.md)
- [Commits](tailwindlabs/tailwindcss@v2.2.14...v2.2.15)

---
updated-dependencies:
- dependency-name: tailwindcss
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
@baileypumfleet baileypumfleet removed the ❗️ changes requested Waiting for the author to implement fixes/suggestions label Sep 21, 2021
.husky/pre-commit Outdated Show resolved Hide resolved
lib/videoClient.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@baileypumfleet baileypumfleet left a comment

Choose a reason for hiding this comment

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

Code looks good to me, will do some local testing now.

.env.example Show resolved Hide resolved
.eslintrc.json Outdated Show resolved Hide resolved
Copy link
Member

@zomars zomars left a comment

Choose a reason for hiding this comment

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

This is awesome! @lunchpaillola

Would you kindly add instructions on how to setup the integration like the other ones in the README.md?

EDIT: I've stumbled upon this error when trying to test it locally:

image

EDIT 2: Also, can we type cast this so we know what properties to expect from the response?

image

@zomars zomars added the ♻️ autoupdate tells kodiak to keep this branch up-to-date label Oct 5, 2021
Lola-Ojabowale added 28 commits October 7, 2021 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
♻️ autoupdate tells kodiak to keep this branch up-to-date ❗️ changes requested Waiting for the author to implement fixes/suggestions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants