Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

docs: revamp exchange files example #1343

Merged
merged 8 commits into from
May 29, 2018

Conversation

fsdiogo
Copy link
Contributor

@fsdiogo fsdiogo commented May 8, 2018

This PR is part of an OKR to level up the Exchange Files example.

Requirements:

  • Level up the design
    • Keep in mind that this is an example and not a full application with all the bells and whistles
    • It should look good but the primary objective is to be educative
    • Keep it familiar as this is used as a template to many projects/examples
  • Update README.md
  • Sync files in workspaces
    • Creates "shared folders" through workspaces named after the url

Example:

  1. User A goes to localhost:8080/#workspace-xpto and drops a file
  2. User B goes to localhost:8090/#workspace-xtpo and sees that the file is there
  3. From this point forward, any file that gets dropped gets shared by the users

Notes:

  • I've taken inspiration from the IPFS UI Kit in terms of colors and components
  • Feedback and suggestions are appreciated 🙂

@fsdiogo fsdiogo self-assigned this May 8, 2018
@fsdiogo fsdiogo requested a review from daviddias May 8, 2018 10:57
@ghost ghost added the status/in-progress In progress label May 8, 2018
@daviddias
Copy link
Member

@fsdiogo I believe it makes sense to update the README and deliver the first iteration as being the revamp of the codebase to then later integrate the sync files. I say this considering the OKRs for Q2.

@fsdiogo
Copy link
Contributor Author

fsdiogo commented May 8, 2018

@diasdavid just to clear things up, do you want me to update the readme considering these changes, and then open another PR implementing the file sync feature?

I thought the file sync feature was part of this OKR.

@fsdiogo
Copy link
Contributor Author

fsdiogo commented May 8, 2018

@diasdavid I've updated the README.

Want me to work on the file sync feat in another branch?

@daviddias
Copy link
Member

Want me to work on the file sync feat in another branch?

Exactly. There is a KR that is a P0 on your list and it should take priority.

@fsdiogo
Copy link
Contributor Author

fsdiogo commented May 9, 2018

@diasdavid alright, I'll start working on that.

When you have the time you can review this PR and I'll make the necessary changes so it can be merged.

})
})
.catch((error) => onError('The inserted multihash is invalid.'))
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this return whatever error came from node.files.get(cid)? This line seems to assume that the error is always that the multihash was wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it assumes the error is an invalid multihash. Before it was returning a stack trace, but in an example app like this I don't think that's the best solution.. neither one are to be honest.

Any suggestions?

Copy link
Member

Choose a reason for hiding this comment

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

The error handling is being handled by @hugomrdias . And after this work being completed, (there is a first PR for several commands, files is not one of them yet) we may forward the received error, since the stacktrace will only be received when in debug mode.
But until then, I think we should provide a friendlier message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, until then I can leave a more general message, something along the lines:

An error occurred when fetching the files.

What do you guys think?

@daviddias
Copy link
Member

image

@fsdiogo
Copy link
Contributor Author

fsdiogo commented May 14, 2018

@diasdavid I removed the start/stop button as we've talked when you were in Porto, it didn't make much sense, it was just cluttering the UI. I'm now starting the node as soon as the window opens and logging that so that the user knows what's happening.

It got stuck probably because the ws-star server needs a reboot, I've already pinged @victorbjelkholm to do that.

I'll change daemon to node 👍

@daviddias
Copy link
Member

@fsdiogo perfect, I'll wait for @victorbjelkholm to give a kick to the Signalling endpoint so that we can fully test this tutorial.

@daviddias
Copy link
Member

@fsdiogo, @victorbjelkholm fixed the issue with WebSockets but the node is still getting stuck on initializing for some reason.

@fsdiogo
Copy link
Contributor Author

fsdiogo commented May 14, 2018

@diasdavid weird... it's working fine here! Can you please try again?

@daviddias
Copy link
Member

@fsdiogo mind rebasing master onto this branch so that CI is green?

@fsdiogo fsdiogo force-pushed the docs/revamp-exchange-files-example branch from 0f77759 to 82d87bb Compare May 15, 2018 17:01
@fsdiogo
Copy link
Contributor Author

fsdiogo commented May 15, 2018

Done @diasdavid.

@fsdiogo fsdiogo force-pushed the docs/revamp-exchange-files-example branch 2 times, most recently from 1eff740 to 7b6d143 Compare May 23, 2018 15:28
@fsdiogo fsdiogo force-pushed the docs/revamp-exchange-files-example branch from 7b6d143 to bd55179 Compare May 25, 2018 16:47
@daviddias daviddias changed the title wip: revamp exchange files example [WIP] revamp exchange files example May 28, 2018
@daviddias
Copy link
Member

@fsdiogo just came to mind that it would be super useful to have a progress bar for file uploads to ipfs. You can get the info with the option progress

@fsdiogo fsdiogo force-pushed the docs/revamp-exchange-files-example branch from 14e9dee to 7283390 Compare May 28, 2018 14:24
@daviddias
Copy link
Member

image

I love it! ❤️

daviddias
daviddias previously approved these changes May 28, 2018
Copy link
Member

@daviddias daviddias left a comment

Choose a reason for hiding this comment

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

LGTM but I would like a review from @alanshaw as well :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants