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

Improvements for Adding to IPFS via Context Menu #585

Merged
merged 8 commits into from
Oct 2, 2018

Conversation

lidel
Copy link
Member

@lidel lidel commented Sep 18, 2018

(Closes #579, #592, #599)

I played a bit with the concept from #579 and it is not enough to change the order.
Instead we should have clear, separate labels for each context. So far I've added a separate menu item for adding link destination and kept common label for images,videos and audio as "Add This Object to IPFS".

Screenshot below illustrates the most extreme case is when you select part of a page and then click on an image that is also a link (so you get actions for all those contexts):

screenshot_28

@alanshaw @olizilla would appreciate quick review of "Add ... to IPFS" menu labels, do they sound ok or awkward? How we could improve it to make English locale better?

Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

Is it possible to distinguish between image/audio/video in the label "Add This Object to IPFS"? Fallback to "object" if unknown/multiple but it would read much better as "Add this image to IPFS" etc.

Also, "This" -> "this"

@alanshaw
Copy link
Member

More thoughts:

  1. What does "add link" do?
  2. Unless there's a good reason to not keep the filename I'd have just a single option for "Add object..." which keeps the filename

@lidel
Copy link
Member Author

lidel commented Sep 19, 2018

Ad #585 (review)) Good ideas. It is possible to provide different label for image, video and audio, so I did just that (see screenshot below).

Ad 1) It executes addFromURL using href from <a> tag that was context-clicked

Ad 2) There are cases when users want raw CID, but I agree it should not be the default.
Removed it for now, not sure how to re-introduce it without duplicating every "Add.." menu item.
Maybe adding "Copy raw CID" + #532 (comment) will solve that use case.

screenshot_8

@alanshaw
Copy link
Member

👍 nice!

How about "Add Linked Content to IPFS" instead of "Add Link Destination to IPFS"?

@olizilla
Copy link
Member

What do "copy canonical" and "copy public gateway" do in this context? I assume they refer to the page not the selection, but it's not clear.

I think it'd be easier to reason about if it was

  • Add selection to IPFS
  • Copy page CID bafy...
  • Copy page IPFS url https://ipfs.io/...

@lidel
Copy link
Member Author

lidel commented Sep 19, 2018

I restored option to get raw CID as a third read-only "Copy.." action.
It resolves IPFS/IPNS path to raw CID, in future it will default to cidv1b32.

Note: until Page Actions are redesigned in #587 the same action labels are used in popu-menus (but don't have to):

Context Menu Page Action (Firefox only) Browser Action
screenshot_50 screenshot_48 screenshot_49

How does this look? Should I move Direct CID below Public Gateway URL? Or rename it?

@olizilla yes, that one was an extreme edge case, usually it is page or image.

Depending on what is clicked, "Copy" actions will look for base URL in this order:

  1. image|video|audio (if clicked on any instead of page)
  2. link (if clicked on any instead of a page)
  3. page

I would not put truncated values in menus, it does not tell much and makes them look cluttered.

However, like you initially suggested do you think we should have separate "Copy.." labels for each context (in context-click menus)?
Eg. "Copy Direct CID for this Image", "Copy Direct CID for this Page" etc?

@olizilla
Copy link
Member

ERH GERD, SORRY I PRESSED THE UPDATE BRANCH BUTTON ACCIDENTALLY.

revoke my permissions at once.

😿

@olizilla
Copy link
Member

anyhoo

ETOOMANYOPTIONS.

Is it possible to add the entire selection to IPFS regardless of what's in it? Could it always be "Add selection to IPFS" even if it's images and text?

"Direct CID" is tricky, as direct doesn't mean anything in this context. I think its more intuitive if we tell the user what the CID will be for (page, selection).

I would not put truncated values in menus, it does not tell much and makes them look cluttered.

It might be a terrible idea, as it could make them look cluttered, but I mildly disagree that it wouldn't tell the user much. Showing what they'll get is really helpful if you don't know what a CID is, or what "canonical" means.

@lidel
Copy link
Member Author

lidel commented Sep 20, 2018

@olizilla after our call I've looked at what Menu API lets us to do and we can add menu separators, icons (only in Firefox, so it the end I disabled them) and submenus.

What if we duplicate "Add.." (every site) and "Copy.." (only IPFS sites) actions per context?

  • Text Selection (no Copy, just Add)
  • Image|Video|Audio (Copy and Add)
  • Link (Copy and Add?)
  • Page (Copy and Add?)

As a PoC I've enabled submenus for every context that had more than one action.
It removed the ambiguity, at least now user knows what is the target of action.

Some examples below:

Multiple Contexts vs Single Context

[selection], image, link selection, [image], link link context alone
screenshot_58 screenshot_63 screenshot_64

Different Contexts

Selection Context Page Context on IPFS site Page Context on normal site
screenshot_62 screenshot_60 screenshot_59

I also renamed "Canonical Address" to "IPFS Path".

Any thoughts on submenus?

@lidel lidel force-pushed the fix/add-image-link-to-ipfs branch from 53990e8 to b6b8638 Compare October 1, 2018 14:49
@lidel lidel merged commit 33f5598 into master Oct 2, 2018
@lidel lidel deleted the fix/add-image-link-to-ipfs branch October 2, 2018 00:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants