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

fix: OS integrations for quick file import #1691

Merged
merged 2 commits into from
Oct 12, 2020

Conversation

lidel
Copy link
Member

@lidel lidel commented Oct 9, 2020

This PR refactors code to fix bugs caused by js api changes.

The sneaky one: the way we pass template changed in a way that failed silently:
https://github.com/ipfs/js-ipfs/blob/8cb8c73037e44894d756b70f344b3282463206f9/packages/interface-ipfs-core/src/object/new.js#L33-L40
so instead of unixfs, we were patching untyped object, which gateway was unable to render.

@rafaelramalho19
I've pushed 1 line fix to your branch and created this PR to include our fixes.

How to test

Same code is used for different variant on each OS, so we need to test both.

macOS

  • drag&drop multiple files or a file and a directory to desktop's icon

Windows

  • right click on a file and select "Add to IPFS" from context menu

Closes #1684

Copy link
Contributor

@jessicaschilling jessicaschilling left a comment

Choose a reason for hiding this comment

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

Successfully dropped onto the menubar icon to import:

  • One file
  • Multiple files
  • A file plus a directory

rafaelramalho19 and others added 2 commits October 9, 2020 23:28
The JS API changed the way template is selected, and we patched untyped
object instead of unixfs-dir one.

Context:
https://github.com/ipfs/js-ipfs/blob/8cb8c73037e44894d756b70f344b3282463206f9/packages/interface-ipfs-core/src/object/new.js#L33-L40

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
@lidel lidel force-pushed the fix/upload-on-file-drag-daemon branch from 4c9f497 to 28879eb Compare October 9, 2020 21:28
@lidel
Copy link
Member Author

lidel commented Oct 9, 2020

@jessicaschilling mind re-testing?

I tested on Windows and works as expected, but noticed that when a single file is imported its filename was lost in the shareable link copied to clipboard.
It was fixed in master, so had to rebase this PR. (sorry!)

@rafaelramalho19
Copy link
Contributor

Looks great! Thanks for picking up from my first bug fix 😄

🚢 itttttttt

Copy link
Contributor

@jessicaschilling jessicaschilling left a comment

Choose a reason for hiding this comment

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

Confirmed import worked and clipboard link displayed successfully for:

  • One file
  • Two files
  • A file plus a directory

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.

"Add to IPFS" OS-level integrations are broken
3 participants