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: helper-ui input-validator update #202

Merged

Conversation

SgtPooki
Copy link
Member

@SgtPooki SgtPooki commented Apr 16, 2024

Title

feat: helper-ui input-validator update

Description

Fixes #66

Notes & open questions

the input validator now accepts the same urls as verified-fetch (more actually since https? is not required) and provides more helpful information

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if necessary (this includes comments as well)
  • I have added tests that prove my fix is effective or that my feature works

the input validator now accepts the same urls as verified-fetch and provides more helpful information
@SgtPooki SgtPooki linked an issue Apr 16, 2024 that may be closed by this pull request
Copy link
Member Author

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

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

self review

@SgtPooki SgtPooki requested a review from a team April 16, 2024 00:27
@SgtPooki SgtPooki self-assigned this Apr 16, 2024
Copy link
Member

@2color 2color left a comment

Choose a reason for hiding this comment

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

From a code perspective looks really clean and correct for all the formats accepted. all the tests.

A couple points regarding UX:

  • The help text with all the accepted formats is overwhelming. My opinion is that we should be a little more prescriptive about the preferred/conventional format while gracefully accepting others (without advertising all of the the graceful cases). The formats with example.com are particularly confusing.
  • I'm somewhat surprised that we don't accept ipfs:// and ipns:// paths. I thought those would be considered the canonical form. I'm also that the /ipfs/.. and /ipns/.. are also common conventions in Kubo.
  • Perhaps instead of adding /path to each example, you omit it from all and update the note to say that you can suffix any path with a path?
Screenshot 2024-04-16 at 3 03 20 pm

@SgtPooki
Copy link
Member Author

The help text with all the accepted formats is overwhelming. My opinion is that we should be a little more prescriptive about the preferred/conventional format while gracefully accepting others (without advertising all of the the graceful cases). The formats with example.com are particularly confusing.

Ok good to know, do you or @lidel have recommended example text we should include here?

I'm somewhat surprised that we don't accept ipfs:// and ipns:// paths. I thought those would be considered the canonical form. I'm also that the /ipfs/.. and /ipns/.. are also common conventions in Kubo.

😲 I missed that. wow. Will add.

Perhaps instead of adding /path to each example, you omit it from all and update the note to say that you can suffix any path with a path?

good advice

@SgtPooki
Copy link
Member Author

Ok I've got the help text as a table, how is this:

image

@lidel
Copy link
Member

lidel commented Apr 16, 2024

It still feels overwhelming. No need to flood user with all variants and permutations.
We just want to hint how valid value type looks like + maybe link to docs.

Maybe simplify closer to:

⚠️ Invalid address, correct it and try again.

ℹ️ For reference, accepted formats are:

  • UNIX-like Content Path (/ipfs/cid/.., /ipns/id/..),
  • HTTP Gateway URL (https://ipfs.io/ipfs/cid.., https://cid.ipfs.dweb.link/..),
  • Native IPFS URL (ipfs://cid/.., ipns://id/..)

Learn more at Addressing IPFS on the Web

@SgtPooki
Copy link
Member Author

Maybe simplify closer to:

Ok will do

@SgtPooki
Copy link
Member Author

that doesn't look much better to me tbh:

image

I think we should have a single example per "Type" we want to show:

image

but there's also some alignment issues with that, so, back to a table but without the header:

image

@lidel
Copy link
Member

lidel commented Apr 17, 2024

Agree, one example per type looks better 👍

Lgtm, but while we are at it, we could improve two things:

  • Rename input label from "CID (and path)" to something like "CID, Content Path, or URL"
    • Since it has "CID", add "CID" type (support pasing standalone CID without /ipfs/ prefix)

@SgtPooki
Copy link
Member Author

  • Rename input label from "CID (and path)" to something like "CID, Content Path, or URL"
    • Since it has "CID", add "CID" type (support pasing standalone CID without /ipfs/ prefix)

Done since 76ae50c & 30e0382

@SgtPooki
Copy link
Member Author

merging due to implicit approvals via emojis on comments

@SgtPooki SgtPooki merged commit e95e70e into main Apr 17, 2024
20 checks passed
@SgtPooki SgtPooki deleted the 66-input-format-validation-doesnt-match-up-with-expected-format branch April 17, 2024 17:44
@lidel lidel mentioned this pull request Apr 17, 2024
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.

Input format validation doesn't match up with expected format
3 participants