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

[Broadcaster] Add chat #13

Merged
merged 1 commit into from
May 29, 2024
Merged

[Broadcaster] Add chat #13

merged 1 commit into from
May 29, 2024

Conversation

mickel8
Copy link
Member

@mickel8 mickel8 commented May 25, 2024

This PR adds chat to the Broadcaster app.
Chat behaviour differs depending on the screen width.
On screens < 1024, the chat is by default hidden and either it or video player can be displayed at the same time.
On screens > 1024, both video player and chat are displayed.

Example screenshots:

  • screen > 1024

image

  • screen < 1024

image
image

@mickel8 mickel8 changed the title Add chat [Broadcaster] Add chat May 25, 2024
@mickel8 mickel8 force-pushed the chat branch 14 times, most recently from 0e8a8c3 to 94bf4c6 Compare May 27, 2024 18:19
@@ -18,12 +18,12 @@ import "./user_socket.js"
// Include phoenix_html to handle method=PUT/DELETE in forms and buttons.
import "phoenix_html"
// Establish Phoenix Socket and LiveView configuration.
import {Socket} from "phoenix"
import {LiveSocket} from "phoenix_live_view"
import { Socket } from "phoenix"
Copy link
Member Author

Choose a reason for hiding this comment

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

vscode formatts this by default. I will try to add linter in a separate PR

@@ -1,6 +1,6 @@
// If you want to use Phoenix channels, run `mix help phx.gen.channel`
// to get started and then uncomment the line below.
import "./user_socket.js"
// import "./user_socket.js"
Copy link
Member Author

Choose a reason for hiding this comment

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

Importing user_socket this way, we count our player panel into the viewers. It could also be tricky to make sure that HTML chat elements are already loaded so I've renamed this file to chat.js, wrapped everything into a function and import this function in home.js.

This way, chat.js is only loaded and used when needed.

@@ -11,7 +11,7 @@
<script defer phx-track-static type="text/javascript" src={~p"/assets/app.js"}>
</script>
</head>
<body class="bg-white antialiased w-screen h-screen flex flex-col">
<body class="bg-white antialiased h-svh flex flex-col">
Copy link
Member Author

Choose a reason for hiding this comment

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

h-svh so we don't have any issues with address bar on mobile phones

@mickel8 mickel8 marked this pull request as ready for review May 27, 2024 18:29
@mickel8 mickel8 requested a review from LVala May 27, 2024 18:30
Copy link
Member

@LVala LVala left a comment

Choose a reason for hiding this comment

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

All good with some nitpicks:

  • pressing enter in the chat input area adds actual enter before the user joins the chat and sends the message after, which I find a bit weird
  • the font in the chat is black which looks a bit weird next to everything else
  • a very tiny scrollbar appears in the chat input when the message gets too long, which also looks a bit weird

@mickel8
Copy link
Member Author

mickel8 commented May 29, 2024

Fixed everything except color. I tried setting nickname to our brand color but body remains black so I am not sure. On the other hand, everything in violet colors is too much, at least for me. I am gonna leave it as it is. Maybe someday someone will make it better

@mickel8 mickel8 merged commit b1e182a into master May 29, 2024
@mickel8 mickel8 deleted the chat branch May 29, 2024 10:54
@LVala LVala mentioned this pull request Jun 12, 2024
54 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants