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

Feat/fe 60 implement agora video call using agora UIKIT #2842

Open
wants to merge 20 commits into
base: dev
Choose a base branch
from

Conversation

taiwonaf
Copy link
Contributor

@taiwonaf taiwonaf commented Dec 10, 2022

new

Fixes Issue/Linear Ticket

( This could be an existing issue or a linear ticket )

My PR Closes #issue_number_here or
My PR Fixes ID-linear_ticket_number_here

  • Team plug (Yieldvest) - Feat/FE-60-Implement-agora-video-call-using-Agora-sdk

Changes proposed

  • Implement Video call using Agora

What were you told to do?

Project 20: Using Agora, make it possible to do a group call in ZuriChat. Also make it possible to do video calls and video broadcasts.

What did you do?

I made it possible to have a video call in a workspace on zurichat using Agora UIKIT. Currently, due to delay in the backend, there is only one channel. When the backend is resolved, there will be different channel, and this will be the workspaces Id

Check List (Check all the applicable boxes)

🚨Please review the style guide for contributing and guidelines for contributing to this repository.

  • My code follows the code style of this project.
  • This PR does not contain plagiarized content.
  • The title and description of the PR is clear and explains the approach.
  • I am making a pull request against the dev branch (left side).
  • My commit messages styles matches our requested structure.
  • My code additions will fail neither code linting checks nor unit test.
Workspace_636831864746182adae96b6b_video.chat.-.Zuri.Chat.-.Google.Chrome.2022-12-10.22-27-33.mp4

@taiwonaf taiwonaf changed the title Feat/fe 60 implement agora video call using agora sdk Feat/fe 60 implement agora video call using agora UIKIT Dec 10, 2022
Copy link
Collaborator

@JulianaSau JulianaSau left a comment

Choose a reason for hiding this comment

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

Instead of adding all those images to the codebase, use the existing icon libraries like react-icons. I'm sure these icons that youre adding already exist there

package.json Outdated Show resolved Hide resolved
@billmal071
Copy link
Collaborator

Like I said before, the icons are ugly, look for better icon, then the format of the video isn't exactly good, if you're not making it half, then put the smaller one on top of the bigger one.

@taiwonaf
Copy link
Contributor Author

Instead of adding all those images to the codebase, use the existing icon libraries like react-icons. I'm sure these icons that youre adding already exist there

I have changed the icon to react icon

Copy link
Collaborator

@JulianaSau JulianaSau left a comment

Choose a reason for hiding this comment

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

Also, if you're not using an icon anymore, delete it entirely. Don't leave it in your commits

const [token, setToken] = useState(null);
const fetchToken = async () => {
const response = await fetch(
`https://staging.api.zuri.chat/rtc/${workspaceId}/publisher/userAccount`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Import BASE_API_URL from Zuri utilities instead of hardcoding it like this. Because there's different base urls for the different development environments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The endpoint is on staging, not on the main yet

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this the zc_core PR or what are you talking about?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The api for fetching the token is currently on zc_core and currently the changes is hosted on staging.api.zuri.chat and not on the base url which is api.zuri.chat

<LiveBroadcast />
</Route>
<div id="zuri-plugin-load-section">
<Switch>
Copy link
Collaborator

Choose a reason for hiding this comment

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

You omitted some very important code up there. Please review and return that loader and the div that you removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So for this, I wrapped the switches inside the zuri plugin load section.

Copy link
Collaborator

Choose a reason for hiding this comment

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

she's referring to the single spa loader

@billmal071
Copy link
Collaborator

@taiwonaf fix conflicts.

@billmal071
Copy link
Collaborator

@JulianaSau

const fetchToken = () => {
axios
.get(
`https://staging.api.zuri.chat/rtc/${workspaceId}/publisher/userAccount`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use the hardcoded variables not this one

setVideoCall(true);
}
})
.catch(error => console.log(error));
Copy link
Collaborator

Choose a reason for hiding this comment

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

The user isn't going to open the log
Handle the error properly

@@ -0,0 +1,3 @@
<svg xmlns="http://www.w3.org/2000/svg" fill="none" viewBox="0 0 24 24" stroke-width="1.5" stroke="currentColor" class="w-6 h-6">
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove this svg file and use the react icons svg

@@ -8,7 +8,7 @@ import { useTranslation } from "react-i18next";

const EditWorkspaceModal = ({ workSpace, editDetails, setEditDetails }) => {
const { t } = useTranslation();

Copy link
Collaborator

Choose a reason for hiding this comment

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

remove stray changes like this and the one you made in the package.json

import styles from "../styles/Sidebar.module.css";
import { useTranslation } from "react-i18next";

// import videoIcon from "../assets/icons/videos.svg";
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove commented out code

Comment on lines -254 to -263
<SingleRoom
name="LiveBroadcast"
image={liveicon}
link={`/workspace/${currentWorkspaceShort}/LiveBroadcast`}
/>
<SingleRoom
name="Voice Call"
image={phoneicon}
link={`/workspace/${currentWorkspaceShort}/voice-call`}
/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Broadcast and voice call are no longer available?

@@ -16,8 +15,16 @@ import {
WorkspaceWrapperStyle
} from "./Workspace.style";

import VideoChat from "../../../components/media-chat/VideoChat";
import VoiceCall from "../../../components/media-chat/VoiceCall/VoiceCall";
// import { GeneralLoading } from "../../../components";
Copy link
Collaborator

Choose a reason for hiding this comment

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

commented code and unnecessary spacing between imports

<LiveBroadcast />
</Route>
<div id="zuri-plugin-load-section">
<Switch>
Copy link
Collaborator

Choose a reason for hiding this comment

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

she's referring to the single spa loader

Comment on lines +103 to +114
{/* <Route
exact
path="/workspace/:workspaceId/marketplace"
component={() => <h1>MarketPlace</h1>}
/> */}

{/* <Route
exact
path="/workspace/:workspaceId/marketplace"
component={() => <h1>MarketPlace</h1>}
/> */}

{/* All other routes not by main go to Single SPA */}
{/* <Route path="/workspace/:workspaceId/*">
<div id="zuri-plugin-load-section">
<p>SHOULD SHOW PLUGINS</p>
</div>
</Route> */}
</Switch>
{/* All other routes not by main go to Single SPA */}
{/* <Route path="/workspace/:workspaceId/*">
<div id="zuri-plugin-load-section">
<p>SHOULD SHOW PLUGINS</p>
</div>
</Route> */}
Copy link
Collaborator

Choose a reason for hiding this comment

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

commented code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants