-
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
Veue 486 audience view show VOD badges when broadcast finishes #311
Conversation
Your Render PR Server URL is https://stage-pr-311.onrender.com. Follow its progress at https://dashboard.render.com/web/srv-c0eg1s3jbvmfvb66u3jg. |
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.
Phew that felt like a lot! Good stuff, couple minor nitpicks, otherwise I see no issue here.
@@ -72,4 +72,8 @@ def seconds_to_time(seconds) | |||
|
|||
[seconds / 3600, seconds / 60 % 60, seconds % 60].map { |t| t.to_s.rjust(2, "0") }.join(":") | |||
end | |||
|
|||
def video_views | |||
current_video.video_views.count |
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.
Tiny note, we use a counter_cache
on video views, instead of count we should be using size
since itll read from the counter_cache first, and then if it cant find it will query from the database to get the count. #count
will always query the database for a count.
https://blog.appsignal.com/2018/06/19/activerecords-counter-cache.html
dataElementName: HTMLElement, | ||
hidden: boolean | ||
hidden: boolean, | ||
visibility: string = "block" |
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.
💡 if you do visibility = "block"
I believe the compiler can auto infer that it should be a string!
|
||
export function currentStreamType(): string | undefined { | ||
const element = document.querySelector("*[data-audience-view-stream-type]"); | ||
return element?.getAttribute("data-audience-view-stream-type"); |
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 always forget we use TS and can use optional chaining!!
@@ -0,0 +1,6 @@ | |||
<svg width="34" height="34" viewBox="0 0 34 34" fill="none" xmlns="http://www.w3.org/2000/svg"> |
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 believe this already exists here!
https://github.com/veuelive/veue/blob/main/app/javascript/images/no-stream.svg
Added by.....checks notes....you!
.widget{ | ||
data: { | ||
"show-when-live": true, | ||
"show-on-audience": true, | ||
} | ||
} |
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.
❓ hmmm...these repeated data-* attributes dont feel great.
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.
@ParamagicDev yeah, but required for show/hide. I think otherwise we need to fetch Markup from backend. 🤔
expect(page).to have_css(".badge-message", visible: true) | ||
|
||
# Hide chat area and show stream ended message when state transition to VOD | ||
expect(page).to_not have_css(".chat-controls") |
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.
Something I did some research on, to_not
will not wait for changes. If you change this to:
expect(page).to_not have_css(".chat-controls") | |
expect(page).to have_no_css(".chat-controls") |
Itll actually wait for changes!
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.
Ah yes, I checked your comment in a PR. Just forgot about it. I'll change it anyway.
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.
After reading through, supposedly to_not
will wait as well? seems like the consensus may still out on that one...but have_no
definitely waits for the driver to timeout
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.
As discussed in #development!
PR Summary
This PR includes working for state change on audience view when broadcast finishes.
This stream has ended
message.