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 homepage field #671

Merged
merged 2 commits into from May 8, 2019

Conversation

Projects
None yet
3 participants
@arp242
Copy link
Contributor

commented May 1, 2019

Add new homepage field so a rel=me link can be added. Also add rel=me to
the Twitter/GitHub links.

See: http://microformats.org/wiki/rel-me

Closes #669

@arp242

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

Travis fails with:

app/validators/http_url_validator.rb:6:23: W: Lint/UriEscapeUnescape: URI.encode method is
 obsolete and should not be used. Instead, use CGI.escape, URI.encode_www_form or 
URI.encode_www_form_component depending on your specific use case.

      uri = URI.parse(URI.encode(value))
                      ^^^^^^^^^^^^^^^^^

CGI.escape() behaves different though; need to check what the easiest way to do this is when I got some time (only needed to make IDN domains work, perhaps there's a better way to start with).

@briankung

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

This answer was pretty helpful for my understanding of the CGI.escape vs URI.encode conundrum: https://stackoverflow.com/a/13059657/1042144

The only problem left is that we didn't escape our query parameters properly, which brings us to the conclusion: we should not use a single method for the entire URI, because there is no perfect solution (so far). As you see & was not escaped from "My Blog & Your Blog". We need to use a different form of escaping for query params, where users can put different characters that have a special meaning in URLs.

arp242 added some commits May 1, 2019

Add homepage field
Add new homepage field so a rel=me link can be added. Also add rel=me to
the Twitter/GitHub links.

See: http://microformats.org/wiki/rel-me

Closes #669
Better validation, fix JS which didn't allow clearing field
Using URI.parse with IDN/non-ASCII URLs won't work, and as far as I can
find there is no stdlib way of doing it. This regexp should be good
enough and not reject any valid URLs.

@arp242 arp242 force-pushed the arp242:homepage branch from a48415f to c709e10 May 7, 2019

@arp242

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

Thanks, I was aware of what the differences/issues are, just not what the solution is.

As far as I can find there is no stdlib way of doing it so I just opted for a regexp validation like the email one. This should be good enough and not reject any valid URLs.

@pushcx

This comment has been minimized.

Copy link
Member

commented May 8, 2019

Looks great. Thanks for being so thorough with styling, js help, and tests, this is wonderful.

@pushcx pushcx merged commit 860d95d into lobsters:master May 8, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@arp242 arp242 deleted the arp242:homepage branch May 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.