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 tel #1429

Merged
merged 9 commits into from
May 19, 2024
Merged

add tel #1429

merged 9 commits into from
May 19, 2024

Conversation

Its-Just-Nans
Copy link
Contributor

add tel detection in href ?

Currently it's

 [IGNORED] tel:xxxx | Unsupported: Error creating request client: builder error for url (tel:xxxx): URL scheme is not allowed

@mre
Copy link
Member

mre commented May 17, 2024

As far as I can see, tel: is not a standardized Uniform Resource Identifier (URI) scheme for phone numbers. At least, I can't find it in https://www.w3.org/wiki/UriSchemes. It's more of an informal standard, am I right?

@mre
Copy link
Member

mre commented May 17, 2024

With that I wanted to say that I like the idea to exclude it, we just do so silently. E.g. we just skip the link entirely. If you like, you can adjust the PR based on this idea.

@mre
Copy link
Member

mre commented May 17, 2024

Oh wow, I did not know that! I'm just blind. 🙈
In this case, all good! Can you add a unit test as well before we merge this?

@Its-Just-Nans
Copy link
Contributor Author

Its-Just-Nans commented May 17, 2024

added some tests

btw I think you're right: tel is not standard in w3c but it is by other consortium like iana (https://www.iana.org/assignments/uri-schemes/uri-schemes.xhtml) and the tel is working almost on every browser (https://caniuse.com/input-email-tel-url)

Copy link
Member

@mre mre left a comment

Choose a reason for hiding this comment

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

I would like to avoid changing the filters, as I can't see a use-case for including phone numbers right now. They can't be checked by lychee and users of the library would be required to set the field.

Apart from that, this is ready to be merged from my side.

lychee-lib/src/client.rs Outdated Show resolved Hide resolved
lychee-lib/src/filter/mod.rs Outdated Show resolved Hide resolved
lychee-lib/src/filter/mod.rs Outdated Show resolved Hide resolved
Its-Just-Nans and others added 4 commits May 19, 2024 14:01
Co-authored-by: Matthias Endler <matthias@endler.dev>
Co-authored-by: Matthias Endler <matthias@endler.dev>
Co-authored-by: Matthias Endler <matthias@endler.dev>
@Its-Just-Nans
Copy link
Contributor Author

Thanks

I guess it's mergeable now

@Its-Just-Nans Its-Just-Nans requested a review from mre May 19, 2024 12:08
Copy link
Member

@mre mre left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request and for being so responsive about the changes!

@mre mre merged commit c3f7fe7 into lycheeverse:master May 19, 2024
7 checks passed
@github-actions github-actions bot mentioned this pull request Oct 6, 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.

2 participants