-
-
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
[CLEANUP beta] Remove bind-attr #11587
Conversation
We should extract this to an addon before merging. Anyone want to volunteer? |
@jasonmit has begun working on the addon: https://github.com/jasonmit/legacy-bind-attr. |
I left a few issues in that repo with some suggestions. |
@@ -101,7 +101,7 @@ QUnit.test('should update bound helpers in a subexpression when properties chang | |||
ignoreDeprecation(function() { | |||
view = EmberView.create({ | |||
controller: { prop: 'isThing' }, | |||
template: compile('<div {{bind-attr data-foo=(dasherize prop)}}>{{prop}}</div>') |
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 test is should update bound helpers in a subexpression when properties change
, however here it has been changed to use an attribute expression and not a subexpression.
Can change this to still use a nested helper? It is specifically testing the subexpression hook.
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.
@mmun - ping
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 is a nested helper: (concat (dasherize prop))
.
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.
I can make it more nested so that it's future-proof.
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.
@mmun I'm considering that putting a mustache in element space results in different hooks being called than putting it in the attr value. With helpers those differences were a constant pain, and I'm presuming this test is specifically for testing a binding in element space.
This is awesome :-D :-D :-D IMO an addon was not required before dropping it (we only committed to addons for view and controller stuff) but it is great to have. 👍 |
Agreed on the addon, but its presence seems good until @code0100fun and @abuiles get a way to auto rewrite templates for us 😃 |
Closed out the two issues and all tests are passing against this branch. I'm ready to transfer ownership of the repository. Thanks |
@mmun - This needs a few small touchup items before this can be merged... |
[CLEANUP beta] Remove bind-attr
No description provided.