-
Notifications
You must be signed in to change notification settings - Fork 31
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
Added Embed player in Asset and Stream page #1295
Conversation
Added the copy code functionality
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Tested the preview and looks awesome! Only suggestion I have is to remove the Download
button from the top right as well (not below the player) so we don't have the same thing in multiple places. WDYT?
|
||
const embedStringForAsset = (playbackId: string) => ` | ||
<iframe | ||
src="https://lvpr.tv?v=${playbackId}" |
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.
Regarding linking to the lvpr.tv player, I think the only real problem from it not using the livepeer.js player yet is that it will not publish any viewership metrics when used, which IMO is highly desired feature. We should update it ASAP to use the SDK player instead.
@adamsoffer I mentioned today that I could update the player to use the SDK player, but after the day today I'm actually not sure when I'll have bandwidth to do that. It'd also involve setting up React from scratch on the project (hopefully a really minimal version of it) and ideally find a way to download/embedded it statically on the IPFS directory, not to have the IPFS website depending on external resources. Do you think anyone from UX team could pick up on that? I can provide all support on the existing stuff in the project, deploying on fleek, etc
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.
No problem! I can pick that up 👍
@victorges Personally, I agree to remove the top-right Download button. It's redundant and also it's not the main CTA, so there's no good reason to have it highlighted IMO. @adamsoffer can I go ahead? |
@clacladev agree let's remove the download button on the top right. |
@adamsoffer @victorges Removed the redundant Download button. Whenever you can review it, then please just approve the PR so I can merge. Cheers 🙌 |
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 awesome. I just pushed a couple very minor fixes (removed the underline text decoration download button and fixed the iframe string format).
What does this pull request do? Explain your changes. (required)
Download
button below the player inside the Asset details pageShare
menu, which then allowsCopy link
andEmbed
dialog, in Assets and Streams details pagesSpecific updates (required)
/dashboard/assets/:id
and/dashboard/streams/:id
page in multiple smaller components. This refactoring was needed to keep the complexity of these pages manageableClipButton
which was implemented a 3 different times in the page in a reusable component.How did you test each of these updates (required)
Does this pull request close any open issues?
Fixes #1282
Screenshots (optional):
The design:
The implementation:
Checklist: