-
Notifications
You must be signed in to change notification settings - Fork 13
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 test and/or little demo #3
Comments
Yeah, a |
@yoshuawuyts 👋 worried that this repo wasn't getting attention I created https://github.com/mapbox/link-hijacker. Pretty much the same thing, with a little influence from other libraries, some options, and some tests. |
Oh dang, haha - I just published a whole bunch more patches; but I like
some of the additions you've added to that repo. It looks like you're not
doing any of the [data-no-routing] stuff are you? Just skipping links
pointing to other hosts + target=_blank? I def dig that.
Would for sure be cool if we could keep our sources somewhat in sync;
perhaps with nanohref being a no-config version of link-hijacker. Anyway,
added you as an owner to nanohref. Thanks!
…On Wed, Jun 28, 2017 at 4:37 PM David Clark ***@***.***> wrote:
@yoshuawuyts <https://github.com/yoshuawuyts> 👋 worried that this repo
wasn't getting attention I created https://github.com/mapbox/link-hijacker.
Pretty much the same thing, with a little influence from other libraries,
some options, and some tests.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACWleh2lP8Ir9r6bBOGSIG9eyFKboqPvks5sImQpgaJpZM4NMW2G>
.
|
This would be awesome! Maybe I should add to link-hijacker 63ef3ce — didn't know about that problem, maybe Besides that point anything else you think would need to be added? If you're interested I could certainly submit a PR making nanohref a no-config wrapper! |
haha, well since we focus on module size a lot, I think having it just be in-source might be better.
No, don't think so! - Thinking of making this a semver major, and adding it to the choo 6 release ✨ choojs/choo#519 |
Also catch |
I'm not sure what you're suggesting here ... would you mind re-stating? |
I don't think wrapping your module is the right way for this module. I
think copying parts of the source is better. Wrapping would create dead
code for this module, which is something we try and avoid (:
…On Thu, Jun 29, 2017, 17:49 David Clark ***@***.***> wrote:
well since we focus on module size a lot, I think having it just be
in-source might be better.
I'm not sure what you're suggesting here ... would you mind re-stating?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACWleozlxxjluwE1JkXVFJTF6uYkURV1ks5sI8drgaJpZM4NMW2G>
.
|
@yoshuawuyts: Ok, I get it, thanks 👍 |
Think we're good to close this. Thanks for the chats! 😁 |
Even though it's a small library, there are still a few ways that the code could go wrong. What do you think about adding some tests?
Or if not automated tests, a little demo you could run for manual testing when creating a PR?
I'm happy to help out with this but don't want to start without getting some sense of your interest and your preferences (in terms of the many runners, frameworks, etc. available, and the general nastiness of getting a good in-browser test all set up).
The text was updated successfully, but these errors were encountered: