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

feat: snapshot link and IPNS path copy options #937

Merged
merged 22 commits into from
Nov 9, 2020
Merged

Conversation

jessicaschilling
Copy link
Contributor

@jessicaschilling jessicaschilling commented Oct 27, 2020

Closes #740.

This PR moves @DeedleFake's work originally in #745 to account for some significant changes to the codebase since then.

  • Adds "Copy Snapshot Link" menu item that generates immutable permalink in the form of https://bafy..foo.ipfs.dweb.link/wiki/
  • Adds "Copy Content Path Snapshot" menu item that generates path in the form of /ipfs/bafy..foo/wiki/
  • Renames existing "Copy IPFS Content Path" menu item to "Copy IPNS Content Path"
  • Clean up hover text for all menu items for better clarity/accuracy
  • Wire up smaller gray preview text for new menu items
  • Move tools options (import, go to node) into header area for semantic clarity (they apply to all of Companion, not just current tab)
  • Check height in Firefox and amend if needed
  • Test: make sure things do/don't display as expected on different kinds of sites

Screenshots

Local page
image

🚨 🚨 Ordinary web2 page (this ain't right!):
image

Gateway page:
image

DNSLink page:
image

Final version (with updated labels):

2020-11-10--00-40-01

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Quick notes on cosmetics:

  • I am worried about information overload on DNSLink/IPNS websites
    • is "permalink" clear enough for regular users? (i have no idea - non-native speaker, so feel free to ignore below notes)
      would users know the difference between "shareable link" and "permalink"?
      • we do, but that is because we know that IPFS is immutable and IPNS is mutable and can provide updates over time.
      • on the old internet "permalink" is just "a link that is promised to work in the future", but here we have a perfect snapshot: is there a better language we could use? would "snapshot link" or "revision link" be bit more explicit? The label should highlight that the link will point to "specific version frozen in time"
  • We should switch our GUIs to copy https://dweb.link/ipfs/{cid} as the canonical URL

ps. some time ago I've added npm run fix:lint:standard which should fix most of linting automatically. perhaps we should just run it as pre-commit hook, so people don't need to worry about fixing it manually?

@jessicaschilling
Copy link
Contributor Author

jessicaschilling commented Oct 28, 2020

@lidel -- some revisions and a few questions:

  1. We're thinking in similar lines about the menu items and helptext. I made some revisions to both in 6925a74 but it may still need tweaking; see updated screenshot at the top of this page, plus commit info, for latest revisions.
  2. Moved the "Pin IPFS Resources" toggle up so it wasn't at the bottom of the list of "Copy" items.
  3. Added npm run fix:lint:standard as a pre-commit hook to our existing Husky setup ... but now I'm getting new and exciting linting errors in CI that never appeared before 🙄
  4. I can't figure out how to get hint text for "Copy Shareable Snapshot" and "Copy Content Path Snapshot" (see screenshot above) to display ... do you mind any advice when you get the chance?
  5. Regarding copying links as https://dweb.link/ipfs/{cid} rather than https://ipfs.io/ipfs/{cid} -- would this just be a matter of using the default public subdomain gateway from prefs instead?

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

This is great!

  1. I made some comments on specific menu items, but overall looks great! Adding Import / My Node buttons made a huge difference! ❤️

  2. iirc we moved pinning to the bottom because people rarely use it, and copy actions are much more useful – I'd remove changes to pinning item from the scope of this PR (rationale in comment inline)

  3. The CI failures are only for pull_request tests – those simulate "how master branch would look like if we merge this PR" and are caused by updated standardjs 15.x rules that landed in master when refactor: webpack 5.x #938 got merged – rebasing this PR should help (but ok to ignore it for now)

  4. resolveToPermalink and resolveToImmutableIpfsPath perform some expensive async operations and both return a promise. I've added missing await in ea9128f (#937) – should be fine now
    (this type of bugs will be removed when we add typescript during rewrite required for What to do about Manifest V3 #666)

  5. It also includes cleanup of historical code which we no longer need, which I just realized makes this a bigger task. I'd say out of scope of this PR – created Replace ipfs.io with dweb.link #939 to track this separately.

add-on/_locales/en/messages.json Outdated Show resolved Hide resolved
add-on/src/popup/browser-action/tools-button.js Outdated Show resolved Hide resolved
add-on/src/popup/browser-action/context-actions.js Outdated Show resolved Hide resolved
add-on/_locales/en/messages.json Outdated Show resolved Hide resolved
@jessicaschilling jessicaschilling marked this pull request as ready for review October 30, 2020 23:23
@jessicaschilling
Copy link
Contributor Author

@lidel -- All items above taken care of, but two remaining concerns:

  1. Getting an "offline" state for ordinary http sites (see updated screenshots in first post)
  2. Also getting rapidly cycling "IPFS API is (Online/Offline)" browser notifications, but can't seem to find any actions in the UI that are triggering these

Do you mind having a look when you are able? Thank you!

lidel added 2 commits November 7, 2020 02:56
This makes path resolution to run asynchronously once, and then from
cache, which makes UI snappy and not blocked by slow HTTP API.

I also swapped labels on Copy X Path menu items to make them more useful
for technical user (regular user will care about shareable/snapshot
links).

There was also change to the pin.ls API, which caused the "offline"
state sometimes (instead of checking specific CID it listed ALL pinned
blocks, which could take some time, during which UI was dead).
@lidel
Copy link
Member

lidel commented Nov 7, 2020

@jessicaschilling landed additional optimizations in b369846 and 6cc79cf – give it a spin in a spare moment :)

Changes:

  • URL-to-Path resolution now runs asynchronously and result is stored in the cache, then value is read from the cache, which makes UI snappy and not blocked by slow HTTP API.
    • we can now reuse cached values during copying, but I've decided it's out of scope (can be a separate PR later)
  • Tested labels a bit and applied some tweaks:
    • Swapped logic a bit to ensure "Copy Shareable Link" is shown on both /ipns/ and /ipfs/
      • "Snapshot" makes sense only in the context of /ipns/
    • Changed path labels back to Copy IPFS/IPNS Path
      • I ended up believing the path form is more useful for technical user: regular user will care about "Shareable/Snapshot links", however technical user will understand content paths labeled as IPFS vs IPNS better.
    • /ipfs/ looks like this:

      ipfs-2020-11-07--03-19-23

    • /ipns/ looks like this:

      2020-11-07--03-19-52

  • There was also change to the pin.ls API, which caused the "offline" state sometimes (instead of checking specific CID it listed ALL pinned blocks, which could take some time, during which UI was dead, just like you described).

@jessicaschilling
Copy link
Contributor Author

@lidel LGTM! Agree on "IPNS Path"/"IPFS Path" in the menu items - hover labels disambiguate this if need be. Please feel free to merge ... thank you so much!

@lidel lidel changed the title feat: add permalink and IPFS path copy options feat: snapshot link and IPNS path copy options Nov 9, 2020
@lidel lidel merged commit bbb2945 into master Nov 9, 2020
@lidel lidel deleted the feat/copy-options branch November 9, 2020 23:03
@lidel
Copy link
Member

lidel commented Nov 9, 2020

2020-11-10--00-40-01

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 "Copy Permalink" Options
2 participants