data-route should not bind click event when target="_blank" is set #677

Merged
merged 3 commits into from Aug 2, 2013

6 participants

@cjoudrey

The use-case here is a <a data-route="something" target="_blank"> within a modal. We are just using the data-route to set the href.

/cc @benwatts

@nickjs
batman.js member

@cjoudrey Status?

@cjoudrey

@nickjs Just pushed a fix per your suggestion.

I started looking into route_rendering_test.coffee, but I'm currently on something else. I think we may be able to test this.

@cjoudrey

In any case, it isn't critical.

@redterror

+1 on this. :)

@airhorns

@cjoudrey any progress on this?

@cjoudrey

@hornairs Haven't looked into this since I initially opened it.

I wasn't sure how we could test this. I could mock Batman.DOM.events.click but feels a little dirty.

Any thoughts?

@nickjs
batman.js member

@cjoudrey You have until Friday to test and ship this :)

@redterror

FWIW - I've been running with this applied to a local copy for months. It works fine. Please merge!

@pushrax
batman.js member

I suppose the base functionality that you want to test is that we don't attach the click handler, so you could check Batman._data(node, 'listeners').click.length and assert it's 0.

@nickjs
batman.js member

Feels dirty to test the implementation. I would trigger a click and just make sure it doesn't do anything.

@pushrax
batman.js member

@cjoudrey I added a test for this, let me know what you think.

@cjoudrey

LGTM 🚢 💨

Thanks for owning this. 👍

@pushrax
batman.js member

@nickjs or @SoapyIllusions if you can once-over this we can finally get it out 🚶

@angelini
batman.js member

Looks good to me.

🎩
🐤

@pushrax pushrax merged commit 9830bc7 into master Aug 2, 2013

1 check passed

Details default The Travis CI build passed
@pushrax pushrax deleted the route-binding-with-target-blank branch Aug 2, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment