Join room #27
Join room #27
Conversation
…cturefeat: add signaling and vid call
Co-authored-by: Srivani Karanth <karanthsrivani@gmail.com> * feat:insert API endpoint * feat: added auto bitrate * feat:insert API endpoint * feat: added auto bitrate
Co-authored-by: SolarPhoenix13 <tisyaagarwal2007@gmail.com> Co-authored-by: Maaya Mohan <maayamohan21@gmail.com>
Co-authored-by: Vanshitha <vanshitha.s@gmail.com>
@mebinthattil @Andy34G7 @Delta18-Git please check this whenever you can |
mebinthattil
left a comment
mebinthattil
left a comment
There was a problem hiding this comment.
I have shared my feedback after a quick scan and tests. Please resolve those issues.
Excited to see where y'all take it from here!
Requesting review from @Delta18-Git & @Andy34G7 as well.
|
Will be reviewing in 30 minutes, thanks for the ping. EDIT: 4 hrs later the review is complete, my bad 😞 |
Delta18-Git
left a comment
Delta18-Git
left a comment
There was a problem hiding this comment.
Feel like its a good start, but since the server code is not present no real way of tracing the logic completely yet. Also quite confusing that Maaya has been the only one working on this.
| <style> | ||
| body { | ||
| background-color: #152238; | ||
| color: white; | ||
| font-family: 'Fjalla One', sans-serif; | ||
| display: flex; | ||
| flex-direction: column; | ||
| align-items: center; | ||
| justify-content: center; | ||
| height: 100vh; | ||
| } | ||
|
|
||
| h1 { | ||
| font-size: 60px; | ||
| margin-bottom: 40px; | ||
| } | ||
|
|
||
| button { | ||
| padding: 14px 28px; | ||
| font-size: 22px; | ||
| border: none; | ||
| border-radius: 10px; | ||
| background-color: #0F52BA; | ||
| color: white; | ||
| cursor: pointer; | ||
| margin-top: 20px; | ||
| transition: background-color 0.3s; | ||
| } | ||
|
|
||
| button:hover { | ||
| background-color: #0c47a1; | ||
| } | ||
|
|
||
| #jamCodeDisplay { | ||
| font-size: 28px; | ||
| margin-top: 20px; | ||
| color: #00ffff; | ||
| } | ||
|
|
||
| #enterRoomHostBtn { | ||
| display: none; | ||
| } | ||
|
|
||
| a { | ||
| position: absolute; | ||
| top: 20px; | ||
| left: 20px; | ||
| color: #00ffff; | ||
| text-decoration: none; | ||
| font-size: 18px; | ||
| } | ||
| </style> |
There was a problem hiding this comment.
I think it makes sense to move the CSS to a separate file which is imported as there is a lot of shared loc in these webpages.
There was a problem hiding this comment.
Same comment applies for all other pages.
updated version with a lighter hover effect as mebin mentioned
another update regarding buttons
There was a problem hiding this comment.
@SolarPhoenix13
I see that you have created two commits both named update index.html, you could have squashed both those commits into a single commit named 'updating index.html to solve excessive glow animation on JamSesh text.'
Just a suggestion for your future commits.
… into server-logic
Room Logic
Which issue(s) does this PR address?
Implementation of room logc
Why do we need this PR?
We need rooms
What logical changes are present in this PR?
Separating client-side code into host and join relevant files.
How did you test the changes in this PR?
I didnt
Are there any breaking changes in this PR?
Maybe
Is there some additional work to be done later that is NOT covered in this PR?
server.js needs some changes to accomodate rooms