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 files page #669

Merged
merged 28 commits into from
Jun 5, 2018
Merged

feat: add files page #669

merged 28 commits into from
Jun 5, 2018

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented May 23, 2018

Start building the files page. This is a minimal version with some controls working: delete, inspect, navigation and file upload. You can also preview music, pdf, video, pictures, audio and text files.

License: MIT
Signed-off-by: Henrique Dias [email protected]

@olizilla
Copy link
Member

👀 now!

Copy link
Member

@olizilla olizilla left a comment

Choose a reason for hiding this comment

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

This is great work @hacdias. Only minor changes needed.

})
}

function runAndFetch ({ dispatch, getIpfs, store }, type, action, args) {
Copy link
Member

Choose a reason for hiding this comment

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

Nice ✨

src/App.js Outdated
'selectRoute',
'doUpdateUrl',
'doInitIpfs',
'doGetGatewayUrl',
Copy link
Member

Choose a reason for hiding this comment

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

doGetGatewayUrl can be removed here, it's not used.

@@ -27,5 +38,21 @@ export default {
}

dispatch({ type: 'IPFS_INIT_FINISHED' })
},

doGetGatewayUrl: () => async ({ dispatch }) => {
Copy link
Member

Choose a reason for hiding this comment

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

doGetGatewayUrl isn't called? I think it's a good plan to have something like this. We might end up with both a "publicGatewayUrl" that points to https://ipfs.io most of the time, and an "localGatewayUrl" that usually points to localhost. I think we've got something like that in companion already.

What you have is good for now. We need to think about how we make this work against a js-ipfs instance, as we connect to window.ipfs, so we can't be sure that the user has http gateway. We could just use the public one in that case, but it would be neat if we could load the users files directly via window.ipfs rather than assuming an http gateway.

Copy link
Member Author

Choose a reason for hiding this comment

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

Uhh, forgot to call it. Now it's called after doInitIPFS. 😄 It doesn't work when using companion though because it doesn't allow .config.get.

Copy link
Member

@lidel lidel May 31, 2018

Choose a reason for hiding this comment

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

We should start thinking on adding window.ipfs in "privileged mode" for WebUI, which provides access to all APIs and has no sandboxing.

Initial idea was to add a mechanism for pre-emptively requesting permissions, as suggested in ipfs/ipfs-companion#454 (comment) and ipfs/ipfs-companion#454 (comment), it could be extended with sandbox flag:

window.ipfs.proxy.acl.whitelist(['config.get'], {sandboxing: false})

The problem is that other dapps could easily disable sandboxing this way.
Not sure if we are able to secure it without some sort of pubkey crypto, which requires us to maintain key for signing webui request for access to privileged APIs.

Is this a blocker or just an inconvenience?
Are you able to work with HTTP API until we sort this out?


const empty = (
<div>
<h2>It seems a bit lonely here :(</h2>
Copy link
Member

Choose a reason for hiding this comment

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

@akrych when you get time, could you create some ideas for a "you have no files yet, why not add some" page for the files view? It could use some of the space to introduce some IPFS or at least some of the issues around "they are only on your machine until a peer fetches them from you", and "this is p2p. this is not a cloud"

}

componentWillMount () {
console.log('WUOLL MOUNT')
Copy link
Member

Choose a reason for hiding this comment

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

WULLY

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't even remember why I added this listeners... 😆


for (const file of raw) {
promises.push(readAsBuffer(file))
console.log(file)
Copy link
Member

Choose a reason for hiding this comment

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

Let's use https://github.com/visionmedia/debug for this kind of logging.

</object>
)
case 'video':
console.log('Ivdeo')
Copy link
Member

Choose a reason for hiding this comment

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

Ivdeo!

default:
const cantPreview = (
<div>
Sorry, we can not preview this file..
Copy link
Member

Choose a reason for hiding this comment

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

There are going to be a bunch of media types we can't preview in a browser... @akrych can you add designs for "preview page for file types where we can't show a preview of the content". We can still ask IPFS for a file size at least, but that isn't much.

More generally, I think that doing a good job of showing a preview for all types of files is a lot of work. Like, many files will say they are are "video" but that doesn't mean we have the codecs to play them. Same for audio.

We could do extra work and unpack zip files and show their structure but then we've got tar,7zip,rar etc which would each need custom previews.

Then there is all the text flavours. Do we go full github and try and render all of them too? W

We need to at least catch all the scenarios where the current browser can't preview a file that might normally be playable. Showing a video element, that just shows a gray box is a bad user experience, and worse than just saying we can't preview it.

We could preview files in browser tabs for now, and let the browser decide what to do with it. It's not great, but it avoids a load of edge cases for until we're ready to do all the work that a robust, general purpose preview page will need.

@lidel @alanshaw @hacdias what are your thoughts on this?

Copy link
Member

@lidel lidel May 31, 2018

Choose a reason for hiding this comment

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

I agree with @olizilla , not sure if we want to go into the rabbit hole of adding file previews of too many formats. It will be time consuming and it could easily be added later via PRs (great opportunity for contributors). That being said, two additional ideas below.

Improving UX without real previews

It may have more sense to rename it to "File Details" (as it does not imply preview is always possible).

Even if we can't play a .mov video or open a .docx document, we can visually communicate to user if a file is a video, document, archive.

To roughly determine if preview can be rendered, we could do proper mime-type sniffing via something like file-type (it only needs the first 4100 bytes of every file).

In absence of real preview for specific format, the screen could display file size, detected mime-type with some human-readable description of the file type, a different Icon for popular mime-types (or just generic image/sound/video/document/archive/unknown) and a Download button to enable user to open file in third-party handler provided by OS.

Previews for web-safe formats

If we want to have minimal support for previews we should aim at low hanging fruits: images, text files and basic audio and video player for formats supported by the browser with a fallback Download button if it can't be played for some reason.


if (!this.state.content) {
this.loadContent()
return <div>Loading</div>
Copy link
Member

Choose a reason for hiding this comment

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

@akrych can you give us some design directions on "page loading" and "component loading" states please.

@@ -12,7 +12,6 @@
"aac",
"mp2",
"mp3",
Copy link
Member

Choose a reason for hiding this comment

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

As we want to look up a file type from an encoding, a map or object would be a better mechanism for storing these lookups.

extToType.js

{
  amr: 'audio',
  bw: 'audio',
  jpg: 'image',
  jpeg: 'image',
  mp4: 'video'
  // ...and on and on
}

This ensures that we don't put the same extension in more than one file and keeps lookups as fast as possible and allows us to easily add new icon types for specific extensions if we need to.

@olizilla
Copy link
Member

The layout for the action items needs to be tweaked, the buttons jiggle depending on how many items are selected.

webui-action-bar-icon-wiggle

Also, I suggest changing the design so that when switching from 1 selected item to 2+, the options that are no longer applicable remain visible, but are disabled and have reduced opacity. We can tooltip them to say "only available for individual files". I'd reorder the options so that the ones that get disabled are to the right hand side. The position of the buttons should remain the same regardless of how many items are selected.

@olizilla
Copy link
Member

Uploading large files kills chrome. We have this issue in companion, and we solve it by blocking the upload and warning the user.

@lidel @alanshaw should we port that logic to window.ipfs? or at lease wrap window.ipfs with it, as any app that tries to do it is going to hit this problem. It's not ideal but I think it's better that we throw an error that the developer can catch rather than letting the tab fail.

@olizilla
Copy link
Member

@akrych the files list looks beautiful, but I'm concerned that the information density is too low for a file list. The user can't see many files in a single screen. Can we tighten it up a little without spoiling the feeling?

@olizilla
Copy link
Member

@hacdias being able to click through to the IPLD explorer is totally rad. I wonder if we can ask MFS if it know the local mfs path for a given CID... that way the IPLD page could offer a link back to the files view if a node is a unixfs one.

@lidel
Copy link
Member

lidel commented May 31, 2018

Uploading large files kills chrome. [..] should we port that logic to window.ipfs?

@olizilla extracted this discussion into separate issue: ipfs-inactive/ipfs-postmsg-proxy#33

@hacdias
Copy link
Member Author

hacdias commented May 31, 2018

@olizilla thanks for your review 😄 I don't think we can take an hash and get its path on MFS right now. There doesn't seem to be an API for that.

@hacdias
Copy link
Member Author

hacdias commented Jun 4, 2018

@olizilla I fixed most of the issues you mentioned. Can we merge this?

@akrych
Copy link

akrych commented Jun 4, 2018

@akrych the files list looks beautiful, but I'm concerned that the information density is too low for a file list. The user can't see many files in a single screen. Can we tighten it up a little without spoiling the feeling?

Yeah, we can tight it up :) I think we can make smaller margins (18px) on the top and bottom of file icon

@olizilla
Copy link
Member

olizilla commented Jun 5, 2018

@hacdias can you fix up the action bar layout jiggle, then we can merge this.

@hacdias hacdias merged commit 2a76554 into revamp Jun 5, 2018
@hacdias hacdias deleted the feat/revamp/files branch June 5, 2018 11:34
@hacdias hacdias mentioned this pull request Jul 9, 2018
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants