Conditional can-EVENT bindings don't work in stache #1650

Closed
akagomez opened this Issue Apr 28, 2015 · 12 comments

Comments

Projects
None yet
6 participants
@akagomez
Contributor

akagomez commented Apr 28, 2015

Demo: http://jsfiddle.net/akagomez/gtt4vf04/

Problem:
Adding a conditional {{#if whatever}} around a can-click="{methodName}" breaks the binding even if the conditional is true.

Test: d218bad

/cc @justinbmeyer @daffl

@moschel

This comment has been minimized.

Show comment
Hide comment
@moschel

moschel Apr 29, 2015

Contributor

I feel like I've filed this and worked on it before. I don't remember what happened to it. I'll investigate.

Contributor

moschel commented Apr 29, 2015

I feel like I've filed this and worked on it before. I don't remember what happened to it. I'll investigate.

@moschel

This comment has been minimized.

Show comment
Hide comment
@moschel

moschel Apr 29, 2015

Contributor

The PR I was thinking of was #1347

Contributor

moschel commented Apr 29, 2015

The PR I was thinking of was #1347

@akagomez

This comment has been minimized.

Show comment
Hide comment
@akagomez

akagomez Apr 29, 2015

Contributor

Darn. I probably could had reused that test and just changed mustache to stache.

Contributor

akagomez commented Apr 29, 2015

Darn. I probably could had reused that test and just changed mustache to stache.

@moschel moschel assigned moschel and unassigned kylegifford Apr 30, 2015

@daffl daffl added this to the 2.2.6 milestone Apr 30, 2015

@kylegifford kylegifford assigned kylegifford and unassigned moschel May 1, 2015

@kylegifford

This comment has been minimized.

Show comment
Hide comment
@kylegifford

kylegifford May 4, 2015

Contributor

@daffl @justinbmeyer I'm a bit stuck on this, could use your input. The contents of the {{#if}} are returned on https://github.com/bitovi/canjs/blob/master/view/stache/mustache_core.js#L114, but for this case, it is something like "can-click="foo" ". I believe what needs to happen is instead of just returning this as a string, it should compile and hydrate the contents of the result. Something like:

var result = rendererWithScope(newScope, newOptions || parentOptions, parentNodeList || nodeList);
if(result) {
    // result.compile().hydrate()
}

Does this seem like the right place to put this code? If so, what method can be used to compile and/or hydrate the result? Also, is there anything that needs to be done to remove the binding once "can-click="foo" " is removed from the DOM? Thanks!

Contributor

kylegifford commented May 4, 2015

@daffl @justinbmeyer I'm a bit stuck on this, could use your input. The contents of the {{#if}} are returned on https://github.com/bitovi/canjs/blob/master/view/stache/mustache_core.js#L114, but for this case, it is something like "can-click="foo" ". I believe what needs to happen is instead of just returning this as a string, it should compile and hydrate the contents of the result. Something like:

var result = rendererWithScope(newScope, newOptions || parentOptions, parentNodeList || nodeList);
if(result) {
    // result.compile().hydrate()
}

Does this seem like the right place to put this code? If so, what method can be used to compile and/or hydrate the result? Also, is there anything that needs to be done to remove the binding once "can-click="foo" " is removed from the DOM? Thanks!

@daffl daffl modified the milestones: 2.2.7, 2.2.6 May 14, 2015

@whitecolor

This comment has been minimized.

Show comment
Hide comment
@whitecolor

whitecolor Jul 22, 2015

Contributor

this still doesn't work in lates minor and this too #1800

Contributor

whitecolor commented Jul 22, 2015

this still doesn't work in lates minor and this too #1800

@akagomez

This comment has been minimized.

Show comment
Hide comment
@akagomez

akagomez Jul 23, 2015

Contributor

@whitecolor The PR hasn't been merged yet.

Contributor

akagomez commented Jul 23, 2015

@whitecolor The PR hasn't been merged yet.

@whitecolor

This comment has been minimized.

Show comment
Hide comment
@whitecolor

whitecolor Jul 23, 2015

Contributor

@akagomez ok, hope to see this soon

Contributor

whitecolor commented Jul 23, 2015

@akagomez ok, hope to see this soon

@daffl daffl closed this in #1724 Jul 24, 2015

@whitecolor

This comment has been minimized.

Show comment
Hide comment
@whitecolor

whitecolor Jul 24, 2015

Contributor

@daffl will this work on minor?

Contributor

whitecolor commented Jul 24, 2015

@daffl will this work on minor?

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Jul 24, 2015

Contributor

I just tried a merge and it turned out to be a huge pain so probably not until two weeks from now.

Contributor

daffl commented Jul 24, 2015

I just tried a merge and it turned out to be a huge pain so probably not until two weeks from now.

@whitecolor

This comment has been minimized.

Show comment
Hide comment
@whitecolor

whitecolor Jul 24, 2015

Contributor

@daffl have a look at this #1800 too please, perchance related.

Contributor

whitecolor commented Jul 24, 2015

@daffl have a look at this #1800 too please, perchance related.

This was referenced Jul 27, 2015

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jul 27, 2015

Contributor

@daffl Did the merge: #1806

Contributor

justinbmeyer commented Jul 27, 2015

@daffl Did the merge: #1806

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Jul 29, 2015

Contributor

I was indeed being complicated. Thanks!

Contributor

daffl commented Jul 29, 2015

I was indeed being complicated. Thanks!

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