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

Do not build url if href starts with 'tel:' #16

Merged
merged 3 commits into from Mar 6, 2016

Conversation

Projects
None yet
4 participants
@julianhandl
Contributor

julianhandl commented Feb 29, 2016

Besides the popular mailto link the tel link gets used more and more. Especially in the age of responsive design it is important to support this kind of link which lets your link trigger a "real" call on your phone or your default communication application (skype, ...) on your desktop pc.

@jensens

This comment has been minimized.

Show comment
Hide comment
@jensens

jensens Feb 29, 2016

Member

+1 but please add a line to the changelog

Member

jensens commented Feb 29, 2016

+1 but please add a line to the changelog

@jensens

This comment has been minimized.

Show comment
Hide comment
@julianhandl

This comment has been minimized.

Show comment
Hide comment
@julianhandl

julianhandl Mar 1, 2016

Contributor

Ready for merge? :)

Contributor

julianhandl commented Mar 1, 2016

Ready for merge? :)

@gforcada

This comment has been minimized.

Show comment
Hide comment
@gforcada

gforcada Mar 1, 2016

Contributor

@julianhandl although @jensens already did it, you can always start a Jenkins job to test that your pull request does not break anything, to do so:

In Plone we value a lot our testing suite, so we usually never merge pull requests if they have not been tested by Jenkins first.

Contributor

gforcada commented Mar 1, 2016

@julianhandl although @jensens already did it, you can always start a Jenkins job to test that your pull request does not break anything, to do so:

In Plone we value a lot our testing suite, so we usually never merge pull requests if they have not been tested by Jenkins first.

@julianhandl

This comment has been minimized.

Show comment
Hide comment
@julianhandl

julianhandl Mar 1, 2016

Contributor

@gforcada Thank you. I didn't know how to start one until now.

Contributor

julianhandl commented Mar 1, 2016

@gforcada Thank you. I didn't know how to start one until now.

@@ -324,6 +324,7 @@ def unknown_starttag(self, tag, attrs):
scheme = urlsplit(href)[0]
if not scheme and not href.startswith('/') \
and not href.startswith('mailto<') \

This comment has been minimized.

@thet

thet Mar 1, 2016

Member

that's not an issue of yours, but isn't the mailto check wrong?
mailto links start with mailto: and not mailto<.

@thet

thet Mar 1, 2016

Member

that's not an issue of yours, but isn't the mailto check wrong?
mailto links start with mailto: and not mailto<.

This comment has been minimized.

@julianhandl

julianhandl Mar 1, 2016

Contributor

Totally right but I thought it was some kind of super special case because it actually works somehow.

@julianhandl

julianhandl Mar 1, 2016

Contributor

Totally right but I thought it was some kind of super special case because it actually works somehow.

This comment has been minimized.

@thet
@thet

thet Mar 1, 2016

Member

This comment has been minimized.

@julianhandl

julianhandl Mar 1, 2016

Contributor

I agree. Don't know why it works but nevertheless it should be corrected to "mailto:".

@julianhandl

julianhandl Mar 1, 2016

Contributor

I agree. Don't know why it works but nevertheless it should be corrected to "mailto:".

This comment has been minimized.

@thet

thet Mar 1, 2016

Member

: and > are close together on english keyboard layouts. that might be the cause :)

@thet

thet Mar 1, 2016

Member

: and > are close together on english keyboard layouts. that might be the cause :)

@thet

This comment has been minimized.

Show comment
Hide comment
@thet

thet Mar 1, 2016

Member

sidenote: I wanted reclaim to add callto: as I did for plone.app.contenttypes, but i found out, that only tel is a standard: http://stackoverflow.com/questions/1164004/how-to-mark-up-phone-numbers and http://tools.ietf.org/html/rfc3966 (page 15).

Member

thet commented Mar 1, 2016

sidenote: I wanted reclaim to add callto: as I did for plone.app.contenttypes, but i found out, that only tel is a standard: http://stackoverflow.com/questions/1164004/how-to-mark-up-phone-numbers and http://tools.ietf.org/html/rfc3966 (page 15).

@julianhandl

This comment has been minimized.

Show comment
Hide comment
@julianhandl

julianhandl Mar 1, 2016

Contributor

Ok. So the build failed because the branch was not found on the remote repo.
I'm going to add the "mailto" fix here. Is that ok?
How do I fix the issue with the branch?

Contributor

julianhandl commented Mar 1, 2016

Ok. So the build failed because the branch was not found on the remote repo.
I'm going to add the "mailto" fix here. Is that ok?
How do I fix the issue with the branch?

@thet

This comment has been minimized.

Show comment
Hide comment
@thet

thet Mar 1, 2016

Member

@julianhandl you just need to have your branch in the plone/plone.outputfilters repo instead of your fork.

can you also fix the mailto issue, run the tests and fix the tests (or add one, if no test is there?). would be awesome.

Member

thet commented Mar 1, 2016

@julianhandl you just need to have your branch in the plone/plone.outputfilters repo instead of your fork.

can you also fix the mailto issue, run the tests and fix the tests (or add one, if no test is there?). would be awesome.

@julianhandl

This comment has been minimized.

Show comment
Hide comment
@julianhandl

julianhandl Mar 1, 2016

Contributor

Ok. Just to be clear: The proper way to develop a fix or enhancement is to checkout the repo itself and create a feature branch instead of creating a fork and do the same there?

Contributor

julianhandl commented Mar 1, 2016

Ok. Just to be clear: The proper way to develop a fix or enhancement is to checkout the repo itself and create a feature branch instead of creating a fork and do the same there?

@gforcada

This comment has been minimized.

Show comment
Hide comment
@gforcada

gforcada Mar 1, 2016

Contributor

@julianhandl you can do both of course, but it's easier to get help from others if you do the branches in plone organization directly.

I usually fork only if I want to make custom changes that either later will be upstreamed or that they belong only to client code.

Contributor

gforcada commented Mar 1, 2016

@julianhandl you can do both of course, but it's easier to get help from others if you do the branches in plone organization directly.

I usually fork only if I want to make custom changes that either later will be upstreamed or that they belong only to client code.

@jensens

This comment has been minimized.

Show comment
Hide comment
@jensens

jensens Mar 1, 2016

Member

@julianhandl We usually (well most of us) branch (not fork) here, like "julianhandl-fix-tel-urls" - but a fork is fine too. A branch has some advantages if it comes to collaborative work.

@gforcada I copy your great PR testing explanation into my FF Clippings Manager for reuse if this is ok

Member

jensens commented Mar 1, 2016

@julianhandl We usually (well most of us) branch (not fork) here, like "julianhandl-fix-tel-urls" - but a fork is fine too. A branch has some advantages if it comes to collaborative work.

@gforcada I copy your great PR testing explanation into my FF Clippings Manager for reuse if this is ok

@thet

This comment has been minimized.

Show comment
Hide comment
@thet

thet Mar 1, 2016

Member

@gforcada it looks like you need a feature branch in the original repository in order to be able to use Jenkins pull-request jobs, no?

Member

thet commented Mar 1, 2016

@gforcada it looks like you need a feature branch in the original repository in order to be able to use Jenkins pull-request jobs, no?

@gforcada

This comment has been minimized.

Show comment
Hide comment
@gforcada

gforcada Mar 1, 2016

Contributor

@thet no, it should work with forks as well, as long as it's not buildout.coredev though. So a pull request from a fork of CMFPlone should work fine, let me know if it's not like that.

@jensens I will write that up and add some screenshots/videos and put it on jenkins.plone.org repository so we can link it. My idea is still to do a one time job run automatically (just like travis is doing when you create a pull request) and then add a comment with instructions on how to run it again (the docs I was mentioning before).

Contributor

gforcada commented Mar 1, 2016

@thet no, it should work with forks as well, as long as it's not buildout.coredev though. So a pull request from a fork of CMFPlone should work fine, let me know if it's not like that.

@jensens I will write that up and add some screenshots/videos and put it on jenkins.plone.org repository so we can link it. My idea is still to do a one time job run automatically (just like travis is doing when you create a pull request) and then add a comment with instructions on how to run it again (the docs I was mentioning before).

@thet

This comment has been minimized.

Show comment
Hide comment
@thet

thet Mar 2, 2016

Member

@julianhandl To add the mailto fix here, is absolutely ok.

Member

thet commented Mar 2, 2016

@julianhandl To add the mailto fix here, is absolutely ok.

gforcada added a commit that referenced this pull request Mar 6, 2016

Merge pull request #16 from julianhandl/telephone-link
Do not build url if href starts with 'tel:'

@gforcada gforcada merged commit 8d07483 into plone:master Mar 6, 2016

1 check was pending

Plone Jenkins CI - pull-request-5.0 Job started, wait until it finishes
Details
@thet

This comment has been minimized.

Show comment
Hide comment
@thet

thet Apr 7, 2016

Member

adressed here: #17

Member

thet commented Apr 7, 2016

adressed here: #17

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment