-
Notifications
You must be signed in to change notification settings - Fork 42
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
Most replayed Heatmap (Logging) #575
Most replayed Heatmap (Logging) #575
Conversation
…t-popular-parts-of-videos
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 am not sure if this PR is reviewable yet. However, I was super interested in how you implemented it, so, if it isn't opened officially let's call this early feedback. 😅
Nevertheless, looks pretty cool 💯
@MatthiasReumann Thank you for your feedback! @MatthiasReumann @joschahenningsen Lets discuss if its probably better to use progressReport to increment section counters for the video :). |
Thanks for your feedback! The testutils you have written are pretty sick!! 🚀 |
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.
Looks great 💯
If this isn't merged before #549 you can even use the **new and improved** testutils. Then testutils.First(...)
and json.Marshal(...)
are not necessary anymore and you can specify the body as
{
...
Body: gin.H{"position": testPosition}
...
}
I've left a comment regarding security concerns. But we might want to discuss that in a meeting.
…t-popular-parts-of-videos
Splitted the heatmap implementation into two parts:
This Pullrequest only contains the logging logic.