-
Notifications
You must be signed in to change notification settings - Fork 27.4k
feat($compile): change directive's restrict setting to default to EA (el... #8321
Conversation
Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.
If you need to make changes to your pull request, you can update the commit with Thanks again for your help! |
I should probably mention that there is a small risk of breaking change because now all directives will match on elements, but because of prefixes it should be very rare that a directive will be matched by an element accidentally. |
@petebacondarwin can you please review? |
@@ -212,6 +212,7 @@ var ngRepeatDirective = ['$parse', '$animate', function($parse, $animate) { | |||
var NG_REMOVED = '$$NG_REMOVED'; | |||
var ngRepeatMinErr = minErr('ngRepeat'); | |||
return { | |||
restrict: 'A', |
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.
Is there any reason why this couldn't be an element too, if we wanted?
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.
it could be but that would be a bit weird.
besides I tried not to introduce new "features" along the way.
expect(log).toEqual('angular'); | ||
} | ||
)); | ||
|
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.
Why is this being removed in this PR?
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 just moved things around and. I should have split it into a separate commit. sorry about that.
It seems like this PR is making a bunch of unrelated changes to the compile tests. Is that right? |
…(element/attribute) Previously we defaulted just to A because of IE8 which had a hard time with applying css styles to HTMLUnknownElements. This is no longer the case with IE9, so we should make restrict default to EA. Doing so will make it easier to create components and avoid matching errors when creating new directives BREAKING CHANGE: directives now match elements by default unless specific restriction rules are set via `restrict` property. This means that if a directive 'myFoo' previously didn't specify matching restrictrion, it will now match both the attribute and element form. Before: <div my-foo></div> <---- my-foo attribute matched the directive <my-foo></my-foo> <---- no match After: <div my-foo></div> <---- my-foo attribute matched the directive <my-foo></my-foo> <---- my-foo element matched the directive It is not expected that this will be a problem in practice because of widespread use of prefixes that make "<my-foo>" like elements unlikely. Closes angular#8321
...ement/attribute)
Previously we defaulted just to A because of IE8 which had a hard time with applying css styles to HTMLUnknownElements.
This is no longer the case with IE9, so we should make restrict default to EA. Doing so will make it easier to create
components and avoid matching errors when creating new directives