feat: preserve file name, support sharing multiple files #479

lidel · 2018-05-16T15:38:34Z

This PR implements #349 by switching to the latest js-ipfs-api which enables support for wrapWithDirectory option in ipfs.files.add.

UI Preview

I did not want to do a redesign so just added options at the bottom:

Not sure if these labels are clear enough, any suggestions?

TODO before merge

Add

support sharing multiple files
add checkbox for controlling directory wrapping
add checkbox for controlling automatic pinning (added while we are at it)
add locale keys

Fix

disable controls when embedded node is used
solve FILE_TOO_LARGE error from mozilla/addons-linter caused by the size of the latest js-ipfs-api

Optional (can be a separate PR)

wait for Feature: Wrap with directory js-ipfs#1329 to be released (js-ipfs > v0.28.2) and switch to it, check fi wrapping works and unlock it for embedded node

Implements #349 (for now only for external node)

This is a simple workaround that encourages factor-bundle to do a more even split that keeps .js files under 4MB limit.

lidel · 2018-05-16T23:17:04Z

@alanshaw @olizilla UI is temporary, but is there anything that could be improved (eg. labels) before we merge it?

Multiple files can be selected using Ctrl/Shift modifier.

olizilla · 2018-05-17T13:02:19Z

"Preserve the filename when adding a single item" is good wording for the label, but it leads to the question, when would I not want that? I vote for just doing the right thing here (wrap) and not troubling the user with the decision, as per ipfs/ipfs-gui#15 (comment) I don't think wrapping by default would harm anyone here.

"Recursively Pin added data"... I think we just do this for all data the user adds. I don't see any harm in doing this and not asking the user.

lidel · 2018-05-17T15:16:29Z

@olizilla Yeah it may be just me 🙃, but I often share ephemeral images and text snippets that don't need a name (or have an ugly one that I'd have to change before sharing) nor persistence (I don't' care to keep it alive forever).

I agree, the defaults are sane, but I really want to give power user an option to opt-out without need of using commandline, so I just collapsed them under a tiny upload options » that reveals them on click:

Or maybe even shorter:

olizilla · 2018-05-17T15:56:50Z

@lidel ok well, you are our archetype power user, and if it helps you, then I'm for it. Collapsing it by default is a good idea, best of both.

olizilla · 2018-05-17T15:57:31Z

"Upload options" is good and clear.

olizilla · 2018-05-18T09:45:22Z

I'll give this a try now

olizilla · 2018-05-18T10:10:18Z

Using unicode glyphs as icons always seems like a good idea, but the cross browser rendering issues will drive you mad. I have tried.

extra lefty ticks on FF on mac

no labels on Chrome on mac

olizilla · 2018-05-18T10:11:04Z

@lidel i'll see if I can track down why the labels are missing on chrome.

olizilla · 2018-05-18T10:43:46Z

@lidel ah, the chrome label issue went away after a rebuild. meh.

Now the labels are hidden by default, I'm thinking they can be more informative like

--wrap-with-directory Wrap files in a directory to preserve their filenames.
--pin Pin files so they are retained when performing garbage collection on your local IPFS repo.

See: https://ipfs.io/docs/commands/#ipfs-add for more info

lidel · 2018-05-18T13:05:18Z

Switched to pure CSS checkmark that should look the same everywhere, updated labels.
I don't want to focus on it too much as the screen will be redesigned anyway,
but it should look ok for now:

@olizilla please check if it does not 🔥 on mac.
I'd like to merge this and release to beta ASAP :)

olizilla

Nice 👍 minor tweaks and good to go. One query about the file opener loop.

olizilla · 2018-05-18T13:31:49Z

add-on/src/lib/ipfs-companion.js

-          'url': url
-        })
-        console.info('[ipfs-companion] successfully stored', path)
+        console.log('uploadResultHandler.file-' + result.indexOf(file), file)


Remove debug logging or add [ipfs-companion] prefix

olizilla · 2018-05-18T13:38:17Z

add-on/src/lib/ipfs-companion.js

+        console.log('uploadResultHandler.file-' + result.indexOf(file), file)
+        // open the wrapping directory (or the CID if wrapping was disabled)
+        if (result.length === 1 || file.path === '' || file.path === file.hash) {
+          await browser.tabs.create({


What's the expected behaviour here? If i add a single file and let it be wrapped, I'm seeing it open the directory not the file. If i add multiple, it shows the directory.

Yes, that is correct.

If wrapping is enabled or you are uploading more than one file, it will always open wrapping directory (the default).
If wrapping is disabled and a single file is uploaded, it will be opened directly. (requires explicit opt-in from user via upload options).

I feel it is a good default for now:

Most of users want to keep file names and as a side effect will get a branded directory listing provided by gateway
(Until we integrate ideas from https://github.com/tableflip/share-via-ipfs via separate PRs)

Uploaded file can be a .zip file and browser can't show that.
(Opening the file itself will just close the tab and start the download, which makes it difficult for user to copy the URL)

Mobile users are not forced to download the data, they just get directory listing and decide what do do with it (open the specific file, copy link to directory, copy direct link to file etc)

olizilla · 2018-05-18T13:41:18Z

add-on/src/popup/quick-upload.js

+    `
+  }
+  return html`
+    <button class='f6 link dim bn bg-transparent moon-gray dib pa0 pointer ma0' style='color: #6ACAD1' onclick=${onExpandOptions}>


Add an mt3 here to get the same vertical space when the button transitions to the options.

feat: Support wrapWithDirectory in Quick Upload

6fc89d6

Implements #349 (for now only for external node)

lidel added the UX label May 16, 2018

fix: workaround FILE_TOO_LARGE via factor-bundle

c6840f7

This is a simple workaround that encourages factor-bundle to do a more even split that keeps .js files under 4MB limit.

lidel changed the title ~~[WIP] feat: preserve file name in Quick Upload~~ feat: preserve file name in Quick Upload May 16, 2018

lidel mentioned this pull request May 17, 2018

UX improvements for sharing local files #342

Closed

11 tasks

feat: Share multiple files at once

35384e1

Multiple files can be selected using Ctrl/Shift modifier.

fix: hide upload options by default

ece5d10

lidel changed the title ~~feat: preserve file name in Quick Upload~~ feat: preserve file name, support sharing multiple files May 17, 2018

fix: remove styling from locale label

0abaf20

style: tweak quick upload options

9d5d388

olizilla requested changes May 18, 2018

View reviewed changes

style: align upload options button

866718f

olizilla approved these changes May 18, 2018

View reviewed changes

fix: add validation of dir-wrapped ipfs.files.add

45ccfca

lidel merged commit 46363c9 into master May 18, 2018

lidel deleted the feat/wrap-with-directory branch May 18, 2018 16:48

This was referenced May 18, 2018

Unable to upload to IPFS API: passing multiple files to ipfs.files.add sometimes fails #480

Closed

Add support for wrapping quick uploads in a directory. #349

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: preserve file name, support sharing multiple files #479

feat: preserve file name, support sharing multiple files #479

lidel commented May 16, 2018 •

edited

Loading

lidel commented May 16, 2018 •

edited

Loading

olizilla commented May 17, 2018

lidel commented May 17, 2018 •

edited

Loading

olizilla commented May 17, 2018

olizilla commented May 17, 2018

olizilla commented May 18, 2018

olizilla commented May 18, 2018

olizilla commented May 18, 2018

olizilla commented May 18, 2018

lidel commented May 18, 2018

olizilla left a comment

olizilla May 18, 2018

olizilla May 18, 2018

lidel May 18, 2018

olizilla May 18, 2018

feat: preserve file name, support sharing multiple files #479

feat: preserve file name, support sharing multiple files #479

Conversation

lidel commented May 16, 2018 • edited Loading

UI Preview

TODO before merge

lidel commented May 16, 2018 • edited Loading

olizilla commented May 17, 2018

lidel commented May 17, 2018 • edited Loading

olizilla commented May 17, 2018

olizilla commented May 17, 2018

olizilla commented May 18, 2018

olizilla commented May 18, 2018

olizilla commented May 18, 2018

olizilla commented May 18, 2018

lidel commented May 18, 2018

olizilla left a comment

Choose a reason for hiding this comment

olizilla May 18, 2018

Choose a reason for hiding this comment

olizilla May 18, 2018

Choose a reason for hiding this comment

lidel May 18, 2018

Choose a reason for hiding this comment

olizilla May 18, 2018

Choose a reason for hiding this comment

lidel commented May 16, 2018 •

edited

Loading

lidel commented May 16, 2018 •

edited

Loading

lidel commented May 17, 2018 •

edited

Loading