-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
Looks good. Did not actually run or test it. There's some minor things.
- non-standard white-spacing ( aligning against token on previous line instead of just depth * indentSize )
- introduces some TODO's in comments -- better to leave them out, note whatever shortcomming, and create an issue for it.
I guess I'd also expect go-ipfs and js-ipfs to just serve the same app, so it can be iterated on as a separate project
test fails bc of some gulp-coverage breakage |
@kumavis Fixed indentation and removed TODO comment |
Dont mind the security warning, its a known situation and is in an outdated copy of the build tool |
@diasdavid thoughts? otherwise LGTM |
I won't have the time to review this soon. Nevertheless one thing that caught my eye is that there aren't any tests. Also, if we are going to push forward and add the gateway to js-ipfs, I would prefer to:
|
One other note, the current code base does not use promises, and we would like to keep it that way for now. |
I think this is another point for breaking the ui out into its own project |
Yeah, there is the webui and this 'file viewer' thing. Both can be their own projects I believe, transforming the file viewer to a NWA instead of a server side rendered thing, so that it can be used easily by go and js-ipfs. |
@diasdavid bit unclear what the decision is for this PR. Should we focus on merging this or should we focus on splitting it out and integrate as external module? |
@diasdavid Something like this? https://github.com/harshjv/ipfb |
@harshjv completed missed the last comment. https://github.com/harshjv/ipfb looks great, that file browser should be something that both go and js can use and not only js. Could you handle these tasks first: I would be comfortable merging this if these tasks were completed. We can figure out how to have 'one file-browser' for both implementations next. It is very probable that by having the Gateway working on js-ipfs, that we will develop an even better file browser since it will enable more JavaScripters to play with it :) Btw, js-ipfs now fully supports paths as in |
@diasdavid @harshjv Hi there, Are there any plans to fix the minor conflict and merge this PR ?? |
@ya7ya see above, there are more things to do than merge conflicts. Wanna help? |
@diasdavid Yeah, I'm currently swamped with work trying to figure out the problem with #946 which is desperately needed for a project i'm working on, but I can try to help by finishing the tasks mentioned in your previous comment |
hey @diasdavid , I been working on the points you mentioned above to get this thing done, But there is an issue I thought you might be able to clarify. The Does |
Add gateway route
/ipfs/*
content-type
when file name is availablehttps://ipfs.io/ipfs/<some-dir-hash>
Preview
Related issue
#693