Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

feat($compile): change directive's restrict setting to default to EA (el... #8321

Closed

Conversation

IgorMinar
Copy link
Contributor

...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

@mary-poppins
Copy link

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

  • Uses the issue template (#8321)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

@IgorMinar IgorMinar added this to the 1.3.0 milestone Jul 24, 2014
@IgorMinar
Copy link
Contributor Author

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.

@IgorMinar
Copy link
Contributor Author

@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',
Copy link
Member

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?

Copy link
Contributor Author

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');
}
));

Copy link
Member

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?

Copy link
Contributor Author

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.

@petebacondarwin
Copy link
Member

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
@IgorMinar IgorMinar closed this in 11f5aee Jul 24, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants