-
Notifications
You must be signed in to change notification settings - Fork 18
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
added tests and root functionality #49
Conversation
@@ -9,14 +9,14 @@ const noRoutingAttrName = 'data-no-routing' | |||
// and url lives on the same domain. Replaces | |||
// trailing '#' so empty links work as expected. | |||
// fn(str) -> null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you change the signature to be:
// (fn(str), obj?) -> null
thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, sorry, good catch! Fixing!
sweet, thanks! |
Gah, apparently forgot to use my brain in that first comment signature fix. Should be better now. :P |
Btw, to be exact, the function returns In addition to the tests in place, I think 👍 |
Whoops again, good catch @jliuhtonen. Agreed on unit tests alone not really feeling sufficient here. Also agreed on dealing with that later. :) |
@@ -8,15 +8,15 @@ const noRoutingAttrName = 'data-no-routing' | |||
// handle a click if is anchor tag with an href | |||
// and url lives on the same domain. Replaces | |||
// trailing '#' so empty links work as expected. | |||
// fn(str) -> null | |||
function href (cb) { | |||
// (fn(str), obj?) -> undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you leave this as null? thanks - I know it's only a convention and not enforced so I know it's a bit silly but yeah I use null
like C's void
return. Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah well, whatever. Merging!
Thanks! I'll publish this at a later time as master isn't published yet. Cheers! |
This PR extends the signature of
href
tohref(cb, root)
, whereroot
is an optional dom node that we will never capture clicks outside of.When adding tests for this I also added missing test for the basic
href
functionality, including the data attribute thing that @jliuhtonen implemented in #27.