Bug with accordion-group and ng-class

[Polooo2]

Hi,
since 0.13.3 I am getting parse errors when using accordion-group in combination with ng-class. This is being caused by this change ead15e3 .

You can test this with following example:

... <accordion-group ng-class="{'test' : isTest}"> ....

This results in following generated code:
... <accordion-group ng-class="{'test' : isTest} {'panel-open': isOpen}"> ....

[stephanie-dm]

I can confirm this behaviour as well. And because of this problem, the class is not added when the panel is open.

[wesleycho]

Using ng-class on a directive that uses replace is a bad idea.

The best solution is to use the template-url option on the accordion-group directive if you want to eliminate that ng-class usage.

[icfantv]

@wesleycho what do you think about adding an expandedClass and/or collapsedClass option to the directive so the fix to #3419 isn't potentially "breaking?" Seems pretty heavy-handed to force someone to make a new template because of a feature we added.

[wesleycho]

I'm not against it necessarily, but in increases cumbersomeness of usage. replace is also a potentially dangerous thing in tandem with another directive that mutates the DOM.

If a PR is opened, I'll gladly review and merge it though.

[icfantv]

Agreed. I recall a comment in an issue in the angular codebase by @caitp indicating that while replace isn't going anywhere, one needs to be cognizant that using it incorrectly can break the DOM.

I wonder if it would be best to establish best practices (or at least "standards") on a project level and just say "If we do x, then we won't support y." In this case it could be: "Because we use replace, to prevent breaking the DOM, we won't support certain, other directives, like ng-class on our components."

[wesleycho]

I can get behind that, as ideally, you should avoid using directives that mutate the DOM in combination with a directive that uses replace.

One can wrap the element with a div that has the desired ng-class to work around this also.

[icfantv]

@wesleycho found this link via #1741. Look at @caitp's last comment. If Angular isn't going to address this, I think we can close this issue and #1741 and add a comment to the docs about not supporting certain directives on UI Bootstrap directives that use replace: true.

[wesleycho]

That settles it for me - closing as won't fix as per resolution in Angular.

[schungx]

Then how can I put a custom class on each accordion group?

For example, I used to use ng-class to add an empty class to the accordion group if its children is empty (therefore the panel-body shouldn't have a top border that takes up 1px, causing a tiny animation whenever that empty accordion group is expanded or collapsed.

I think there are real needs for ng-class to work.

[pkozlowski-opensource]

I think there are real needs for ng-class to work.

No one argues about it. It is just a technical solution that is missing here. If you've got an idea on how to solve it from the technical point of view, a pull request would be appreciated.

[wesleycho]

One can use class="{{customClass()}}" to force a custom class according to custom rules.

[schungx]

@wesleycho, works perfectly. Thanks!

[malibeg]

Can someone post example of class="customClass()}}

[icfantv]

@malibeg, please do not use the issues forum for support related requests. Rather, please follow the instructions here.

Additionally, please do not comment on closed issues as it is a very good way for us to miss stuff.

[hoxu]

Using ng-class on a directive that uses replace is a bad idea.

The best solution is to use the template-url option on the accordion-group directive if you want to eliminate that ng-class usage.

@wesleycho instead of requiring the users of angular-bootstrap to read the source code and know internal implementation details (use of replace and ng-class element on root of the template), why can't you just add additional div to the template that does not have ng-class?

Rather than requiring library users to know which directives use replace and ng-class or other conflicting builtin directives, I think library designers should avoid using those conflict-prone directives on the root element of the templates.

Additionally, please do not comment on closed issues as it is a very good way for us to miss stuff.

@icfantv With all due respect for great work on the library, in my experience issues get closed before getting feedback from the reporter, so it's quite hard not to comment on closed issues.

[wesleycho]

@hoxu the problem here is that using ng-class is absolutely recommended against in this exact situation, so even if we did somehow allow ng-class usage here, you will unexpectedly add 4 watchers instead of the desired 1. The pain is much more preferred than allowing the user to do something carelessly unperformant.

In this case, one needs to be familiar with how replace works when using angular, and given that the library uses it (in the future we will remove replace support as it is deprecated), we cannot support ng-class usage as per the angular team's own decision on this situation.

Going to lock discussion on this issue, as what has been said is pretty straightforward & has not been contested - there is no need for further commenting & pinging all prior users.