-
Notifications
You must be signed in to change notification settings - Fork 9
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
Feature/chat room isolation and fixes #6
Conversation
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 mean, total lack of tests ain't great... but we're getting there! Approved!
@@ -13,7 +13,7 @@ def create | |||
end | |||
|
|||
ActionCable.server.broadcast( | |||
"chat_room", | |||
"live_video_#{message.video_id}", | |||
{text: message.body, user_name: current_user.full_name, video_id: message.video_id}, |
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.
Do you need to pass the video id here as a param also? It is already going to the channel for it
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.
Yes, I'll take it and will try to eliminate it. Actually for video_id
, we are using it inside received(data)
of consumer.subscriptions.create
. Another way is to take roomId
from params like this JSON.parse(this.identifier).roomId
roomId: this.videoId |
${data.text} | ||
</label> | ||
</div> | ||
` |
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.
It's like low-complexity JSX 😀
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.
yeah without render
function.... 😄
@@ -1,3 +1,7 @@ | |||
@import '../vars'; | |||
@import '../utils/text_style'; | |||
@import '../utils/grid_style'; |
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 show you in a refactor PR how to use these with modules!
No description provided.