-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Make actions use target=this
implicitly
#14479
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,11 +15,13 @@ | |
with | ||
|
||
```handlebars | ||
<button {{action this 'foo'}}> | ||
<button onblur={{action this 'foo'}}> | ||
<button onblur={{action this (action this 'foo') 'bar'}}> | ||
<button {{action target=this 'foo'}}> | ||
<button onblur={{action target=this 'foo'}}> | ||
<button onblur={{action target=this (action target=this 'foo') 'bar'}}> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this also backwards. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes; it has been corrected. |
||
``` | ||
|
||
If an action already has a target it is left unmodified. | ||
|
||
@private | ||
@class TransformActionSyntax | ||
*/ | ||
|
@@ -41,16 +43,19 @@ TransformActionSyntax.prototype.transform = function TransformActionSyntax_trans | |
ElementModifierStatement(node) { | ||
if (isAction(node)) { | ||
insertThisAsFirstParam(node, b); | ||
ensureTarget(node, b); | ||
} | ||
}, | ||
MustacheStatement(node) { | ||
if (isAction(node)) { | ||
insertThisAsFirstParam(node, b); | ||
ensureTarget(node, b); | ||
} | ||
}, | ||
SubExpression(node) { | ||
if (isAction(node)) { | ||
insertThisAsFirstParam(node, b); | ||
ensureTarget(node, b); | ||
} | ||
} | ||
}); | ||
|
@@ -62,6 +67,24 @@ function isAction(node) { | |
return node.path.original === 'action'; | ||
} | ||
|
||
function ensureTarget(node, builders) { | ||
if (!hashPairForKey(node.hash, 'target')) { | ||
let thisTarget = builders.pair('target', builders.path('this')); | ||
node.hash.pairs.push(thisTarget); | ||
} | ||
} | ||
|
||
function hashPairForKey(hash, key) { | ||
for (let i = 0; i < hash.pairs.length; i++) { | ||
let pair = hash.pairs[i]; | ||
if (pair.key === key) { | ||
return pair; | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function is unfortunately repeated. @rwjblue should the util function be DRY somewhere? And if so where? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will extract it and then make another PR to update other refs. |
||
|
||
return false; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function is unfortunately repeated. @rwjblue should the util function be DRY somewhere? And if so where? |
||
|
||
function insertThisAsFirstParam(node, builders) { | ||
node.params.unshift(builders.path('this')); | ||
} |
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.
This isn't quite right, the hash has to be after positional args...
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.
TIL...