Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

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

Merged
merged 3 commits into from

6 participants

Christian Joudrey Nicholas J. Small Dave Steinberg Harry Brundage Justin Li Alex Angelini
Christian Joudrey
Collaborator

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

Nicholas J. Small
Owner

@cjoudrey Status?

Christian Joudrey
Collaborator

@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.

Christian Joudrey
Collaborator

In any case, it isn't critical.

Dave Steinberg

+1 on this. :)

Harry Brundage
Collaborator

@cjoudrey any progress on this?

Christian Joudrey
Collaborator

@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?

Nicholas J. Small
Owner

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

Dave Steinberg

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

Justin Li
Owner

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.

Nicholas J. Small
Owner

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

Justin Li
Owner

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

Christian Joudrey
Collaborator

LGTM :ship: :dash:

Thanks for owning this. :+1:

Justin Li
Owner

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

Alex Angelini
Owner

Looks good to me.

:tophat:
:baby_chick:

Justin Li pushrax merged commit 9830bc7 into from
Justin Li pushrax deleted the branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
9 src/view/bindings/route_binding.coffee
View
@@ -13,13 +13,14 @@ class Batman.DOM.RouteBinding extends Batman.DOM.AbstractBinding
super
+ return if @onAnchorTag && @node.getAttribute('target')
Batman.DOM.events.click(@node, @routeClick)
routeClick: (node, event) =>
- return if event.__batmanActionTaken
- event.__batmanActionTaken = true
- params = @pathFromValue(@get('filteredValue'))
- Batman.redirect(params) if params?
+ return if event.__batmanActionTaken
+ event.__batmanActionTaken = true
+ params = @pathFromValue(@get('filteredValue'))
+ Batman.redirect(params) if params?
dataChange: (value) ->
if value
16 tests/batman/view/route_rendering_test.coffee
View
@@ -8,6 +8,7 @@ QUnit.module 'Batman.View route rendering',
class @App extends Batman.App
@layout: null
@route '/test', ->
+ @route '/', ->
Batman.redirect = @redirect = createSpy()
teardown: ->
@@ -21,12 +22,21 @@ asyncTest 'should set href for URL fragment', 1, ->
QUnit.start()
@App.run()
-asyncTest 'should redirect when clicked', 1, ->
+asyncTest 'should redirect when clicked', 2, ->
@App.on 'run', =>
helpers.render '<a data-route="\'/test\'">click</a>', {}, (node) =>
helpers.triggerClick(node[0])
delay =>
- deepEqual @redirect.lastCallArguments['/test']
+ ok @redirect.called
+ deepEqual @redirect.lastCallArguments, ['/test']
+ @App.run()
+
+asyncTest 'should not redirect when clicked if target attribute is set', 1, ->
+ @App.on 'run', =>
+ helpers.render '<a data-route="\'/test\'" target="_blank">click</a>', {}, (node) =>
+ helpers.triggerClick(node[0])
+ delay =>
+ ok not @redirect.called
@App.run()
asyncTest 'should set "#" href for undefined keypath', 1, ->
@@ -230,7 +240,6 @@ asyncTest 'should redirect to named route queries when clicked', 1, ->
asyncTest 'should allow you to nested elements with route declarations', 6, ->
@App.resources 'products', ->
@collection 'search'
- @App.root ->
@App.on 'run', =>
source = '''
@@ -263,7 +272,6 @@ asyncTest 'should allow you to nested elements with route declarations', 6, ->
asyncTest 'should not stop events from bubbling', 2, ->
@App.resources 'products'
- @App.root ->
@App.on 'run', =>
source = '''
Something went wrong with that request. Please try again.