Channel UI code refactoring #36
Conversation
Consolidates: - Loading spinner - SendMessage controls - Channel mode indicator
| @@ -168,9 +168,9 @@ class Channel extends React.Component { | |||
| ChannelActions.sendMessage(this.state.channelName, text); | |||
| } | |||
|
|
|||
| sendFile(filePath: string, buffer) { | |||
| sendFile(filePath: string, buffer, mimeType = 'application/octet-binary') { | |||
There was a problem hiding this comment.
I think it would be nice to have an optional mime type stored in file post metadata (ipfs-post would need to support it). Maybe even allow to pass in further metadata as a generic object?
There was a problem hiding this comment.
We could add the mimeType to ipfs-post which is what defines the data structure for each message: https://github.com/haadcode/ipfs-post/blob/master/src/FilePost.js and https://github.com/haadcode/ipfs-post/blob/master/src/Post.js.
There was a problem hiding this comment.
Do you want to add that or should i? I'm not sure if we should have an explicit mimeType property or just a more generic object that we can use for more meta data like encoding and whatever else might be useful in the future, i.e. { name, hash, size, meta = { mimeType, encoding } } vs. { name, hash, size, mimeType }
There was a problem hiding this comment.
Definitely prefer the proposed meta field. Added it to ipfs-post (orbitdb-archive/ipfs-post@1224898).
You can use it by adding meta field to you data object in sendFile()
const data = {
...
meta: { encoding: ..., ... }
}
|
Not sure if tests are supposed to pass? |
|
FYI for const readNetworkInfo = (hash) => {
return new Promise((resolve, reject) => {
// catFromJsIpfsApi(hash).then(resolve)
// .catch((e) => {
// catFromJsIpfs(hash).then(resolve)
// .catch((e) => {
logger.warn(".cat - no api or content found, using mock")
resolve(JSON.stringify({
// name: 'localhost dev network',
name: 'Orbit DEV Network',
// publishers: ['localhost:3333']
publishers: ['178.62.241.75:3333']
}));
// });
// });
});
}; |
|
@claus Really good work so far! I won't be able to review this until next week but will come back to you with feedback and comments. |
| const { messages, username, channelName, loading, loadingText, reachedChannelStart, appSettings } = this.state; | ||
| const { colorifyUsernames, useEmojis, useMonospaceFont, font, monospaceFont, spacing } = appSettings; | ||
| const elements = messages.map(message => ( | ||
| <Message |
There was a problem hiding this comment.
In general, let's use parenthesis around function parameters to make it explicit that it's a function:
const elements = messages.map((message) => ...)
|
There are some things to address in the PR, mainly the file catting part and some stylistics things. The tests are not supposed to pass in |
|
|
||
| import Logger from 'logplease'; | ||
| const logger = Logger.create('Clipboard', { color: Logger.Colors.Magenta }); | ||
|
|
||
| var getFileUrl = apiurl.getApiUrl() + "/api/cat/"; | ||
| function readUTF8String(bytes) { |
There was a problem hiding this comment.
As per my original comment on this, could you elaborate on why we need this conversion? Iirc IPFS return everything "in the correct encoding". Perhaps there's a point somewhere down the line (ie. outside the UI) where the encoding happens/is defined and we can replace all this with something more simple (and this might be related to how getFile() is implemented in Orbit.js).
There was a problem hiding this comment.
Well it returns a binary blob, if i'm not mistaken? Or rather, cat returns a Promise that resolves to a binary stream, and in getFile() i currently return a Promise that resolves to a binary blob (i'm aware that this won't stay as it won't work well for large files, but that's not relevant to this particular discussion). A binary blob is just binary data. To convert that to a string (once we established that it contains text), we need to assume some encoding, and most text files nowadays are encoded as UTF-8, so i'm using this function to convert from UTF-8 encoded binary to JS string.
There was a problem hiding this comment.
You are of course right. I'll rework what i did there, also wrt to video streaming etc. js-ipfs cat of course gives me a readable stream that emits chunks that are Buffers, which have a .toString method which handles UTF-8 just fine.
|
Had to resolve a bunch of conflict with recent changes, so I merged your branch with another and PRed that one. Closing this one, but the changes made here are in master now. |
No description provided.