-
Notifications
You must be signed in to change notification settings - Fork 736
AHOYAPPS-40: Add Functionality to Join/Leave a Room #5
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
Conversation
|
|
||
| Run `npm install` to install all dependencies. | ||
|
|
||
| To run the token server, you will need to create a `.env` file (example below) to store your Twilio credentials. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@timmydoza A note for the future: hopefully twilio-cli will help generate this .env file using a script.
| token?: string, | ||
| options?: ConnectOptions | ||
| ) { | ||
| const [room, setRoom] = useState<Room>(new EventEmitter() as Room); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@timmydoza Why do we need to create an EventEmitter here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first I was using const [room, setRoom] = useState<Room>(); and the initial value of room was undefined. This was causing me to use null checks in any component or hook that uses room, which I found to be annoying.
I asked Charlie about this one and we decided that it would be good to use an "empty" room object as an initial value so that we wouldn't have to perform so many null checks all over the place. EventEmitter did the trick since room inherits from that. Someday we may make aclass EmptyRoom extends EventEmitter { ... } if we need to.
For now, I'll assign this to a variable with a descriptive name to make things a little clearer:
const emptyRoom = new EventEmitter() as Room;
charliesantos
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a quick pass. Still wrapping my head around it.
Also, some parts of the PR template are not used?
Please don't forget to rebase later.
| console.log(`issued token for ${token.identity} in room ${req.body.room}`); | ||
| }); | ||
|
|
||
| app.listen(4000, () => console.log('token server running on 4000')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: Can you declare this above as a constant and read from an environment variable with 4000 as default?
const PORT = parseInt(process.env.PORT, 10) || 4000;
...
...
app.listen(PORT, () => console.log(`token server running on${PORT}`));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could but then it wouldn't work with the react dev server. Right now the token comes from the /token endpoint, which is proxied to localhost:4000 per a setting in package.json. I can't set the port in package.json from an environment variable, so I don't think I should do it in server.js.
Plus, since we will end up using a twilio-cli solution for token minting, server.js is just temporary. I only included it so I can develop, and so that other people can start the app.
| import Menu from './components/Menu/Menu'; | ||
| import LocalVideoPreview from './components/LocalVideoPreview/LocalVideoPreview'; | ||
| import useRoomState from './hooks/useRoomState/useRoomState'; | ||
| import { useVideoContext } from './hooks/context'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: can we sort the imports alphabetically? Destructured imports could be last.
It'd be great if we can also add this rule to eslint.
import LocalVideoPreview from './components/LocalVideoPreview/LocalVideoPreview';
import Menu from './components/Menu/Menu';
import useRoomState from './hooks/useRoomState/useRoomState';
import { styled } from '@material-ui/core/styles';
import { useVideoContext } from './hooks/context';
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do this for other files as well.
| import CircularProgress from '@material-ui/core/CircularProgress'; | ||
| import { getToken, receiveToken } from '../../store/main/main'; | ||
| import useRoomState from '../../hooks/useRoomState/useRoomState'; | ||
| import { useVideoContext } from '../../hooks/context'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like how we're grouping the imports by types. Can we separate the group with newlines, and also sort alphabetically? Something like
import * as React from 'react';
import { useDispatch } from 'react-redux';
import AppBar from '@material-ui/core/AppBar';
import Button from '@material-ui/core/Button';
import CircularProgress from '@material-ui/core/CircularProgress';
import Toolbar from '@material-ui/core/Toolbar';
import TextField from '@material-ui/core/TextField';
import { createStyles, makeStyles, Theme } from '@material-ui/core/styles';
import useRoomState from '../../hooks/useRoomState/useRoomState';
import { useVideoContext } from '../../hooks/context';
import { getToken, receiveToken } from '../../store/main/main';
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for other files please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll make these adjustments by hand. Couldn't get the eslint to work so I'll wait on that for now.
0386b18 to
5e4b998
Compare
VIDEO-5730 | Moved more menu to the center of the menu bar


This PR adds the following features:
Kevin's designs are not yet finished, so the appearance of these features will need to be revisited when the designs are complete. So don't mind how things look or where things are. My main goal was to add the functionality.
Some of the design decisions (use of VideoContext and VideoContextProvider, for instance) were made with the idea that we may some day create a React component library for the video SDK. Let me know if you want to hear my thoughts on that.
Contributing to Twilio