-
Notifications
You must be signed in to change notification settings - Fork 413
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
Add FileRender component #1576
Add FileRender component #1576
Conversation
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.
Small changes
const icon = doesPlayback ? 'Play' : 'Folder'; | ||
const label = doesPlayback ? 'Play' : 'View'; | ||
const icon = doesPlayback ? 'Play' : 'Eye'; | ||
const label = doesPlayback ? 'Play' : 'Preview'; |
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.
Can you change these to use the icon constants?
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.
done 👍
src/renderer/constants/icons.js
Outdated
@@ -26,3 +26,5 @@ export const HEART = 'Heart'; | |||
export const UNLOCK = 'Unlock'; | |||
export const CHECK_SIMPLE = 'Check'; | |||
export const GLOBE = 'Globe'; | |||
export const EYE = 'Eye'; |
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.
This prompted: #1683
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.
(not necessary to resolve before merging this)
Initial step for #620 |
|
||
class VideoPlayer extends React.PureComponent { | ||
static MP3_CONTENT_TYPES = ['audio/mpeg3', 'audio/mpeg']; | ||
static FILE_MEDIA_TYPES = ['3D-file', 'e-book', 'comic-book']; |
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 comic-book
an actual media 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.
Oh I'm guessing this is just for future file types.
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.
See #1377
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.
Left a few more comments, not sure if all of them warrant changes.
Tom and I will be doing more testing on this tomorrow, with a lot of file types. Should be able to get it merged in the next day or two. Great work!
const noMetadataMessage = 'Waiting for metadata.'; | ||
const unplayableMessage = "Sorry, looks like we can't play this file."; | ||
const hideMedia = this.playableType() && !hasMetadata && !unplayable; | ||
const unsupportedMessage = "Sorry, looks like we can't preview this file."; | ||
const isLoadingFile = !fileSource && this.fileType(); |
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.
Since we are calling this.fileType()
3 times in 5 lines, lets just set it to a variable and use that for these checks below.
{isLoadingMetadata && <LoadingScreen status={noMetadataMessage} />} | ||
{isUnplayable && <LoadingScreen status={unplayableMessage} spinner={false} />} | ||
{unsupported || | ||
(isUnsupported && <LoadingScreen status={unsupportedMessage} spinner={false} />)} |
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 think this would be easier to follow if we had a shouldDisplayLoadingScreen()
function or something.
const [isLoading, loadingStatus] = this.shouldDisplayLoadingScreen()
Then we would just need one LoadingScreen
component
|
||
class VideoPlayer extends React.PureComponent { | ||
static MP3_CONTENT_TYPES = ['audio/mpeg3', 'audio/mpeg']; | ||
static FILE_MEDIA_TYPES = ['3D-file', 'e-book', 'comic-book']; |
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.
Oh I'm guessing this is just for future file types.
// Render custom viewer: FileRender | ||
if (this.fileType()) this.renderFile(); | ||
// Render default viewer: render-media (video, audio, img, iframe) | ||
else if (this.supportedType()) { |
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.
Should this be an if, else if, else
statement instead?
It looks like it could call renderAudio
and the default viewer both.
- Update fileRender logic - Fix missing file source
- Add new icon Eye - Show preview button
reduce lbry.getMedia calls update fileRender logic
unblock unplayable files fix context menu errors of pdfViwer
Changes
Todo
video
->FileViewer