-
-
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
[FEATURE] override attribute bindings #10213
[FEATURE] override attribute bindings #10213
Conversation
var split = binding.split(':'); | ||
var property = split[0]; | ||
var attributeName = split[1] || property; | ||
|
||
Ember.assert('You cannot use class as an attributeBinding, use classNameBindings instead.', attributeName !== 'class'); | ||
|
||
if (property in this) { | ||
this._setupAttributeBindingObservation(property, attributeName); | ||
if (!attributeHash.hasOwnProperty(attributeName)) { |
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.
Should I only test this if Ember.FEATURES.isEnabled("feature-name")
is true?
If so, which feature name should I use?
Idea: Call this function (wrapped in an ember feature flag) before _normalizeAttributeBindings: function(attributeBindings){
var attributeHash = {};
for (var i = attributeBindings.length-1; i >= 0; i--) {
var binding = attributeBindings[i];
var split = binding.split(':');
var property = split[0];
var attributeName = split[1] || property;
if (!attributeHash.hasOwnProperty(attributeName)) {
//this attribute wasn't overriden, so it's safe to process it
//add to hash to know wether or not we processed it later
attributeHash[attributeName] = property;
} else {
//overriden attribute binding, remove it
attributeBindings.splice(i, 1);
}
}
}, |
@miguelcobain for reference, any changes to the public API should be wrapped in a feature flag. |
Is this feature considered a change in the public API? |
@miguelcobain it introduces a behavior change to the API, no? (By change, I don't mean a breaking change, I just mean that it can now do something differently than it did before.) |
Agreed. On it. A qua, 21/01/2015, 22:37, Peter Wagenet [email protected] escreveu:
|
Well, I just added the test with no changes to the views. Tests are passing! 😕 |
@miguelcobain - The |
@rwjblue Great! I think this is ready be merged. |
…dings [FEATURE] override attribute bindings
Awesome, thanks! |
Adds attribute binding override support.
Related with #10176.
Is there something missing? This is my first contribution attempt. :)
Maybe we should do something similar for classNameBindings.
I could make this override behaviour more abstract, but that would require to loop the attribute array again.
This way we only loop it once.
Should I wrap this behing a feature flag?