-
Notifications
You must be signed in to change notification settings - Fork 16
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
Feat: Video player v1 #703
Conversation
✅ Deploy Preview for testitori ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for teritori-dapp ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
e0060f4
to
b47cd78
Compare
|
||
func (s *VideoService) GetVideoList(ctx context.Context, req *videopb.GetVideoListRequest) (*videopb.GetVideoListResponse, error) { | ||
limit := req.GetLimit() | ||
if limit <= 0 { |
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.
We should cap the limit to avoid to request too much of data
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.
Caped at 40
(Have to handle pagination ?)
import { mustGetVideoClient } from "../../utils/backend"; | ||
import { VideoMetaInfo, VideoInfoWithMeta } from "../../utils/types/video"; | ||
import { useSelectedNetworkId } from "../useSelectedNetwork"; | ||
export type VideosList = { |
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.
is that normal that we have definition of video list type or null ? it means that pages can be: array of null ? please check if we can implement differently or this is the only efficient way to do this.
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 ddin't check the videos hooks, will check later
} as VideoInfoWithMeta); | ||
}); | ||
|
||
const totalCount = 1000; // test |
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.
is this normal we keep hardcoded test value there ?
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.
Need to handle pagination... Will do later
@WaDadidou are all files pushed? I'm getting some errors on files that it cannot find. |
198c96b
to
f50a949
Compare
6c455a3
to
7541035
Compare
c167742
to
7ce3a90
Compare
ed9ef6a
to
1cbe259
Compare
… from video-player indexer
942ed7a
to
f4e99c6
Compare
5883eaf
to
00bad8a
Compare
ae595d0
to
a5765a2
Compare
a5765a2
to
e9b9543
Compare
It's a Draft⚠️ So yes, it may be broken for now 😇
The original PR is #649. The feature frontend and backend were started by @Saturn-Radius. Thank you mate 😄
Need testnet backend to allow testing. We can only test on local env for now. -> See the original's PR https://github.com/TERITORI/teritori-dapp/pull/649description
TODO
I hope it's not a mess