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

Add pin function #29

Closed
wants to merge 1 commit into from
Closed

Add pin function #29

wants to merge 1 commit into from

Conversation

jedahan
Copy link

@jedahan jedahan commented Dec 15, 2015

Please do not merge this, I just opened this to start a discussion. We could come up with a much better ux for pinning / unpinning I think:

  1. make the current button act as pin/unpin, with ON/OFF being replaced with a pin icon. Right click the button to turn on and off the redirect, which would color/uncolor the logo.
  2. create a second button just dedicated to pinning
  3. make the button just pin/unpin, and ask people to enable/disable the addon like they do all other addons, in the addons preference pane.

All of these require better code:

  1. Convert open new tab to ajax call that handles success/failure
  2. Add unpin functionality

And probably want to not hard-code the :5001 port for the daemon (make it the default, allow for override like for the gateway)

Anyway, is this a feature people would like me to work on more?

@@ -8,6 +8,7 @@ var cm = require('sdk/context-menu');
var prefs = require('sdk/simple-prefs').prefs;
var ioservice = Cc['@mozilla.org/network/io-service;1'].getService(Ci.nsIIOService);
var gui = require('./gui.js');
var tabs = require("sdk/tabs");
Copy link
Member

Choose a reason for hiding this comment

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

@lidel
Copy link
Member

lidel commented Dec 15, 2015

Thanks! Looks very good, feel free to work on it 👍

My thoughts:

  • API port should be configurable (just in case someone runs it on something else than 5001)
  • I think pinning via right-click context menu is fine for now (we can improve it later, see below)
    • Is it possible to pin recursively?
  • I agree we should improve UX related to the toolbar button:
    • Right now it simply enables/disables forwarding to local gateway.
    • My plan was to change it to display a menu (on click) with multiple actions instead (perhaps we could simply attach items from context menu?)

@lidel
Copy link
Member

lidel commented Dec 17, 2015

FYI I merged other pull request (#28) and you probably will need to rebase your changes on top of current master.

function pin(address) {
let IPFS_API_URI = 'http://' + prefs.customGatewayHost + ':5001/api/v0';
let uri = ioservice.newURI(IPFS_API_URI+'/pin/add?arg='+address, null, null);
tabs.open(uri.spec);
Copy link
Member

Choose a reason for hiding this comment

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

I think opening this in a Sidebar would be less-intrusive for a user (as a quick UX fix).

Ideally, we would want to parse the response and display some kind of notification instead.

@lidel
Copy link
Member

lidel commented Dec 18, 2015

As for the ToggleButton improvements, this is what I have in mind:

The popup panel would have all items from the context menu (copy link, pin resource etc) + on/off switch for entire plugin (current onClick behaviour).

@jedahan jedahan mentioned this pull request Dec 18, 2015
@lidel
Copy link
Member

lidel commented Dec 18, 2015

Closing this one, will continue in #30

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.

2 participants