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: IPNS Publishing #1857

Closed
wants to merge 0 commits into from
Closed

feat: IPNS Publishing #1857

wants to merge 0 commits into from

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Sep 20, 2021

Closes #1482.

  • Add IPNS key management UI to Settings page
  • Publishing to IPNS id via context action on Files page

A bunch of screenshots

Screen Shot 2021-09-24 at 13 29 10

The context menu is a bit unaligned. The same happens with the Pinning Services context menu. The target reference seems correctly set so I'm not sure why's this miscalculation happening.

Screen Shot 2021-09-24 at 13 29 27

Screen Shot 2021-09-24 at 13 37 16

Screen Shot 2021-09-24 at 13 37 27

Screen Shot 2021-09-24 at 13 37 35

Screen Shot 2021-09-24 at 13 37 50

image

@hacdias hacdias temporarily deployed to Deploy September 20, 2021 13:49 Inactive
@hacdias hacdias temporarily deployed to Deploy September 20, 2021 14:46 Inactive
@hacdias hacdias temporarily deployed to Deploy September 20, 2021 15:24 Inactive
@hacdias hacdias temporarily deployed to Deploy September 20, 2021 15:50 Inactive
@hacdias hacdias temporarily deployed to Deploy September 20, 2021 16:19 Inactive
@hacdias hacdias temporarily deployed to Deploy September 20, 2021 17:05 Inactive
@hacdias hacdias temporarily deployed to Deploy September 20, 2021 17:12 Inactive
@hacdias hacdias marked this pull request as ready for review September 24, 2021 11:40
@hacdias hacdias requested a review from lidel September 24, 2021 11:40
@hacdias hacdias self-assigned this Sep 24, 2021
@hacdias hacdias temporarily deployed to Deploy September 24, 2021 11:44 Inactive
@hacdias
Copy link
Member Author

hacdias commented Sep 27, 2021

This one is ready @lidel :D

@lidel lidel temporarily deployed to Deploy November 24, 2021 22:56 Inactive
@lidel lidel temporarily deployed to Deploy November 25, 2021 01:12 Inactive
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.

Finally, got to spend some time with this :)
Works really good, want to include it in go-ipfs 0.11 if possible.

Before we merge this, need to add some e2e tests tomorrow:

  • add key and confirm pet name is added to the table
    • publish to the key from files
      • confirm link on the settings screen got updated
    • remove key and confirm it's gone from the table

For now, pushed small changes which tweak some modals a bit:

Screenshot 2021-11-25 at 02-07-29  animals Files IPFS

Screenshot 2021-11-25 at 02-10-31 Settings IPFS

Screenshot 2021-11-25 at 02-10-00 Settings IPFS

src/bundles/ipns.js Outdated Show resolved Hide resolved
@lidel lidel changed the title feat: IPNS key management on Settings feat: IPNS Publishing Nov 25, 2021
@lidel lidel mentioned this pull request Nov 25, 2021
@lidel lidel temporarily deployed to Deploy November 26, 2021 11:23 Inactive
@lidel lidel temporarily deployed to Deploy November 27, 2021 00:39 Inactive
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.

Added E2E tests and cleaned them up a bit, should be pretty robust now that we use playwright.


@hacdias I think we have UX gap around publishing, which blocks this from being shipped.

Need some sort of confirmation that publishing was successful.

Visual feedback on IPN Publish

Right now I click Publish and nothing happens.
We need three states:

  • "Publishing in progress"
  • "Publishing successful" / "Publishing failed"

A nice flow would be if clicking on "Publish" displayed spinner until ipfs.name.publish returns result (success/error), and then:

  • if error, show error inside of the modal.
  • if success, show "Succes, published at {keyid}" and below the same UI we have on "Share" modal, where user can copy a shareable link to /ipns/ address at a public gateway.

@hacdias will you have bandwidth to add this?
I think it is better to wait and polish the experience, so I'm descoping this from go-ipfs 0.11, so there is no rush.

@hacdias hacdias temporarily deployed to Deploy November 30, 2021 08:31 Inactive
@hacdias
Copy link
Member Author

hacdias commented Nov 30, 2021

@lidel I'll work on it.

@hacdias
Copy link
Member Author

hacdias commented Nov 30, 2021

@lidel it's still missing the actual spinner. Could you just validate if the workflow is good?

@hacdias hacdias temporarily deployed to Deploy November 30, 2021 11:17 Inactive
@lidel lidel temporarily deployed to Deploy December 2, 2021 23:13 Inactive
@lidel lidel temporarily deployed to Deploy December 2, 2021 23:40 Inactive
@lidel
Copy link
Member

lidel commented Dec 3, 2021

@hacdias yes, the user flow ending with "sharing screen" and "Copy" action is perfect 👌

Figuring out an engaging spinner will be tricky – some thoughts:

  • IPNS publishing can take up to a minute atm because (iirc) it puts IPNS record in 20 peers, and it has to find the right ones first. (It is faster if Accelerated DHT Client is enabled, but can't use it by default yet).
  • A fake progressbar that assumes to take ~1m should be enough of feedback / setting expectations. We can improve it later.
  • We don't have tracing API to see actual IPNS record puts as they happen (yet). If you want to get creative, we could inform user about DHT traversal, eg. show random DHT walk with await first(ipfs.log.tail()) or fake it for now. Goal is to communicate to user that "IPNS publishing is in progress, may take big long"

ps. fysa pushed two changes:

  • Finished E2E tests for IPNS publishing – they now confirm IPNS record was updated by doing ipfsn.name.resolve.
  • Added some copy text above the shareable link. (just a draft, when we are done we will ask a native speaker to proofread and refine messaging and UI)

    Screenshot 2021-12-03 at 00-35-33  animals Files IPFS

@hacdias
Copy link
Member Author

hacdias commented Dec 3, 2021

A fake progressbar that assumes to take ~1m should be enough of feedback / setting expectations. We can improve it later.

I will do this. Seems to be the easiest. Using a spinner would probably not be the greatest idea idea, indeed, as it does not give feedback.

@hacdias
Copy link
Member Author

hacdias commented Dec 3, 2021

@lidel I added a progress bar that is initially 60 seconds. After each publishing, we average the current expected time with the new time it take. I think that this way, it should improve and have better expectations over time.

@hacdias hacdias temporarily deployed to Deploy December 3, 2021 15:01 Inactive
@hacdias hacdias temporarily deployed to Deploy December 3, 2021 15:05 Inactive
@hacdias hacdias requested a review from lidel December 17, 2021 12:16
@hacdias

This comment has been minimized.

@lidel lidel added the status/blocked Unable to be worked further until needs are met label Dec 17, 2021
@lidel lidel temporarily deployed to Deploy December 17, 2021 22:53 Inactive
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.

@hacdias really nice!

  • I adjusted the rolling average to overshoot rather than undershoot (apps fake progress and usually finish around 75% to give that "snappy feeling", we guess based on past times, which is better, but still should overshoot to avoid being stuck at 100%) + added brief explainer what is happening, so the wait is a bit more justified:

    2021-12-17_23-47

useEffect(() => {
if (!start) return

const interval = setInterval(() => {
Copy link
Member

Choose a reason for hiding this comment

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

💭 nit: I wonder if we could remove setInterval here and use css animations instead somehow? (would look less choppy, we could also make it more engaging by making it non-linear).

I'm thinking setting progress to 100% and using animation: progressBar ${expectedPublishTime}s ease-in-out for customizing the progress animation instead.

Just an idea, feel free to ignore.

@hacdias hacdias temporarily deployed to Deploy December 20, 2021 11:45 Inactive
@hacdias
Copy link
Member Author

hacdias commented Dec 20, 2021

@lidel I added an option that will make ProgressBar use CSS animations. I also added an overflow: hidden to the parent div so that the colored part/progress does not overflow.

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.

@hacdias I don't believe ipfs/kubo#8591 or ipfs/kubo#8586 will happen this quarter, let's not wait anymore. I think it is ok for us to ship it because IPNS publishing should be in IPFS Desktop, and unlike the upstream kubo (go-ipfs), IPFS Desktop has IPNS over pubsub enabled by default, so performance will be acceptable.

Mind resolving conflicts in this PR and pinging me when ready? 🙏

@lidel lidel added this to the v2.16 milestone Jul 2, 2022
@SgtPooki
Copy link
Member

SgtPooki commented Jul 9, 2022

@juliaxbow this is a great candidate for your review and help!

@hacdias hacdias force-pushed the feat/1482-settings branch 3 times, most recently from 50f9c8a to 857bdf5 Compare July 10, 2022 06:39
@hacdias hacdias closed this Jul 10, 2022
@hacdias hacdias mentioned this pull request Jul 10, 2022
@hacdias
Copy link
Member Author

hacdias commented Jul 10, 2022

I tried a different of rebasing and I guess I messed up this PR. Sorry for that and won't happen again. The branch is fine, but this PR got closed and I can't re-open it.

Continue on #1973.

@hacdias hacdias removed this from the v2.16 milestone Jul 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/blocked Unable to be worked further until needs are met
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Support for publishing to IPNS
3 participants