LBRYCommunity/lbry-desktop

Fork 0

Add FileRender component #1576

Merged

btzr-io merged 16 commits from file-preview into master

2018-07-10 16:38:24 +02:00

btzr-io commented

2018-06-11 08:44:31 +02:00

(Migrated from github.com)

Changes

Fix Review isPlayable() function #1590
Use router pattern for rendering file viewer https://github.com/lbryio/lbry-app/issues/1544
Initial step for https://github.com/lbryio/lbry-app/issues/620
Add pdf-viewer to actually test something. https://github.com/lbryio/lbry-app/issues/1701

Todo

Improve file type detection.
Fix getMediaType logic https://github.com/lbryio/lbry-redux/pull/51
Rename: video -> FileViewer

### Changes - Fix Review isPlayable() function #1590 - Use router pattern for rendering file viewer https://github.com/lbryio/lbry-app/issues/1544 - Initial step for https://github.com/lbryio/lbry-app/issues/620 - Add pdf-viewer to actually test something. https://github.com/lbryio/lbry-app/issues/1701 ### Todo - [x] Improve file type detection. - [x] Fix getMediaType logic https://github.com/lbryio/lbry-redux/pull/51 - [x] Rename: `video` -> `FileViewer`

neb-b (Migrated from github.com) requested changes 2018-06-18 23:18:57 +02:00

neb-b (Migrated from github.com) left a comment

Small changes

src/renderer/component/video/internal/play-button.jsx

neb-b (Migrated from github.com) commented

2018-06-18 23:18:51 +02:00

Can you change these to use the icon constants?

btzr-io (Migrated from github.com) reviewed 2018-06-19 00:22:31 +02:00

src/renderer/component/video/internal/play-button.jsx

btzr-io (Migrated from github.com) commented

2018-06-19 00:22:25 +02:00

done 👍

done :+1:

kauffj (Migrated from github.com) reviewed 2018-06-25 16:16:40 +02:00

src/renderer/constants/icons.js

kauffj (Migrated from github.com) commented

2018-06-25 16:15:53 +02:00

This prompted: https://github.com/lbryio/lbry-app/issues/1683

kauffj (Migrated from github.com) reviewed 2018-06-25 16:16:58 +02:00

src/renderer/constants/icons.js

kauffj (Migrated from github.com) commented

2018-06-25 16:16:58 +02:00

(not necessary to resolve before merging this)

btzr-io commented

2018-06-27 18:13:33 +02:00

(Migrated from github.com)

Initial step for https://github.com/lbryio/lbry-app/issues/620

neb-b (Migrated from github.com) reviewed 2018-06-28 06:34:06 +02:00

src/renderer/component/video/internal/player.jsx

neb-b (Migrated from github.com) commented

2018-06-28 06:34:06 +02:00

Is comic-book an actual media type?

Is `comic-book` an actual media type?

neb-b (Migrated from github.com) requested changes 2018-06-28 07:18:32 +02:00

neb-b (Migrated from github.com) left a comment

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!

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!

src/renderer/component/video/internal/player.jsx

neb-b (Migrated from github.com) commented

2018-06-28 07:13:46 +02:00

Oh I'm guessing this is just for future file types.

src/renderer/component/video/internal/player.jsx

neb-b (Migrated from github.com) commented

2018-06-28 07:16:17 +02:00

Should this be an if, else if, else statement instead?

It looks like it could call renderAudio and the default viewer both.

Should this be an `if, else if, else` statement instead? It looks like it could call `renderAudio` and the default viewer both.

src/renderer/component/video/internal/player.jsx

neb-b (Migrated from github.com) commented

2018-06-28 06:35:44 +02:00

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.

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.

neb-b (Migrated from github.com) commented

2018-06-28 07:09:47 +02:00

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

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