Client redesign #97
Client redesign #97
Conversation
|
@alir128 UI looks fantastic! There's only one problem though. I don't know if you realized it, but pausing the audio and replaying adds latency between source and client. While that itself is not a problem, WebRTC tries to catch up the client to the source by speeding up the audio. So, do we want this? |
client-redesign/src/views/User.vue
Outdated
| <card /> | ||
| </div> | ||
| <div style="padding: 5px"> | ||
| <connectionStatus /> |
client-redesign/src/views/User.vue
Outdated
| volume: 50 | ||
| }), | ||
| components: { | ||
| card, |
There was a problem hiding this comment.
Components have a CapitalCase naming convention when declared in JS and then kabab-case for their html templates.
There was a problem hiding this comment.
The CapitalCase goes for files as well.
There was a problem hiding this comment.
Sure will address this.
client-redesign/src/views/User.vue
Outdated
| @@ -0,0 +1,51 @@ | |||
| <template> | |||
There was a problem hiding this comment.
I think this file could be renamed to Home.vue
client-redesign/src/router/index.js
Outdated
| component: User | ||
| }, | ||
| { | ||
| path: "/about", |
There was a problem hiding this comment.
Let's remove the routes we're not using.
client-redesign/src/router/index.js
Outdated
| component: () => | ||
| import(/* webpackChunkName: "about" */ "../views/About.vue") | ||
| }, | ||
| { path: "/:id", component: User } |
There was a problem hiding this comment.
The way they parse an id here is how we should be parsing for room names
There was a problem hiding this comment.
I see that you have code inside roomname.vue that checks for route params but you don't check for route params in the home route. I think if you did so, then things will work.
There was a problem hiding this comment.
I can rename :id to :room, but toonin through url by entering the room name is working as shown in the screenshot also that I posted.
| @@ -0,0 +1,98 @@ | |||
| /* eslint-disable no-console */ | |||
There was a problem hiding this comment.
Excellent work on structuring the VueX store!
| toonin() { | ||
| this.connectToRoom(); | ||
| }, | ||
| connectToRoom() { |
There was a problem hiding this comment.
I think the RTC code should be moved over to the global store in the future. A component should ideally not be aware of so much WebRTC complexity.
There was a problem hiding this comment.
There's state management going on here. Maybe I don't understand the code you've written enough but from a prelim glance I feel that these events that modify state could be handled elsewhere.
There was a problem hiding this comment.
Do you want me to move the RTC logic to main.vue, I can’t move it the store as it doesn’t have access to socket io and so it became complicated to subscribe to socket events. And no state management isn’t actually taking place here only Vuex Actions are being called, which call mutations inside the store which is how it mentions to do in Vuex documentation. Component methods -> Vuex actions -> Vuex Mutations -> State updated -> getters used to map state back inside component.
There was a problem hiding this comment.
I will try to use Vue plugin functionality to store webRTC calls and sockets.
@Lakshya2610 Is this happening on this new web app only or did we have this issue before also? I didn't rework the code behind webrtc and the stream so not sure if this issue has to do with anything in this PR. If not then maybe we can open a separate issue for that. |
#92
All 6 issues mentioned before are now resolved.