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

feat: add VOD detail page #1199

Merged
merged 20 commits into from
Aug 17, 2022
Merged

feat: add VOD detail page #1199

merged 20 commits into from
Aug 17, 2022

Conversation

0xcadams
Copy link
Member

@0xcadams 0xcadams commented Aug 11, 2022

What does this pull request do? Explain your changes. (required)

Adds VOD detail page.

Does this pull request close any open issues?

Fixes #1026
Fixes #1027
Fixes #1028
Closes #1024

@vercel
Copy link

vercel bot commented Aug 11, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
livepeer-studio ✅ Ready (Inspect) Visit Preview Aug 17, 2022 at 11:03PM (UTC)

@0xcadams 0xcadams requested review from adamsoffer and a team August 12, 2022 16:08
@0xcadams 0xcadams marked this pull request as ready for review August 12, 2022 16:08
Copy link
Member

@adamsoffer adamsoffer left a comment

Choose a reason for hiding this comment

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

Looks fantastic. Just a few minor change requests.

  • Let's remove the "ready" badge once a video is done uploaded. That badge should only appear if an upload is processing.

image

  • Let's bump up the size of this "Edit Asset" button one unit to match the others

image

  • Add an max width + ellipsis to asset title

image

@victorges
Copy link
Member

Really nice! From testing the UI:

  • The bitrate seems to always be showing 0.00. Does it happen on your assets as well? If yes, maybe we have a bug in the pipeline that is not populating this and we should probably remove it from the frontend as well until that works.
  • The edit asset metadata field has an example object that is incompatible with the schema since it only supports string values right now. Could make the tokenId property value by a string version "316" (can reevaluate schema as well)
    Screen Shot 2022-08-16 at 21 05 23
  • When an asset is not on IPFS yet, there's a button saying "Upload to IPFS". I'd refrain from using the Upload word here not to be confused with the "upload" term for creating new assets. WDYT of Save to IPFS?

@victorges
Copy link
Member

victorges commented Aug 17, 2022

The bitrate seems to always be showing 0.00. Does it happen on your assets as well? If yes, maybe we have a bug in the pipeline that is not populating this and we should probably remove it from the frontend as well until that works.

Ah I found what it is! After #1154, we started hiding some details info from the asset object by default unless requested (not for admins tho). This includes the tracks field inside the videoSpec which has info about the video, where you take the bitrate and resolution from.

So to fix this, you need to add ?details=1 query-string to the API where you get the asset details to show in the page. Value can be anything actually, we only check if it is a truthy string (can be improved).

@0xcadams 0xcadams mentioned this pull request Aug 17, 2022
@0xcadams
Copy link
Member Author

0xcadams commented Aug 17, 2022

Moving TODOs from #1194 to this PR since this is a superset of it:

  • Make warning on upload snackbar yellow/orange instead of a red
  • Add warning while trying to close the tab when uploading

@codecov
Copy link

codecov bot commented Aug 17, 2022

Codecov Report

Merging #1199 (7969e2a) into master (8949cbc) will increase coverage by 0.07209%.
The diff coverage is n/a.

❗ Current head 7969e2a differs from pull request most recent head 9aa7c40. Consider uploading reports for the commit 9aa7c40 to get more accurate results

Impacted file tree graph

@@                 Coverage Diff                 @@
##              master       #1199         +/-   ##
===================================================
+ Coverage   51.39884%   51.47093%   +0.07209%     
===================================================
  Files             66          66                 
  Lines           4325        4317          -8     
  Branches         777         774          -3     
===================================================
- Hits            2223        2222          -1     
+ Misses          1825        1820          -5     
+ Partials         277         275          -2     
Impacted Files Coverage Δ
packages/api/src/controllers/asset.ts 55.44554% <0.00000%> (+0.78316%) ⬆️
packages/api/src/store/asset-table.ts 89.65517% <0.00000%> (+3.44826%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae8791b...9aa7c40. Read the comment docs.

Impacted Files Coverage Δ
packages/api/src/controllers/asset.ts 55.44554% <0.00000%> (+0.78316%) ⬆️
packages/api/src/store/asset-table.ts 89.65517% <0.00000%> (+3.44826%) ⬆️

@0xcadams
Copy link
Member Author

@adamsoffer @victorges @iameli thanks for the review, made those changes requested

Copy link
Member

@adamsoffer adamsoffer left a comment

Choose a reason for hiding this comment

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

Looks great! Just noticed on light mode the close button on the snackbar should be black. Other then that, good to merge.
image

@0xcadams 0xcadams merged commit c5424d9 into master Aug 17, 2022
@0xcadams 0xcadams deleted the 0xcadams/vod-detail branch August 17, 2022 23:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants