fix(accordion): add attr.openClass || 'panel-open' class when expanded #4198
Conversation
uses toggleClass in isOpen watch Fixes angular-ui#4193
| scope.$watch('isOpen', function(value) { | ||
| element.toggleClass(scope.openClass, value); |
There was a problem hiding this comment.
This is incorrect - if the element has a different class for some reason, this will cause a bug.
You will want
element[value ? 'addClass' : 'removeClass'](scope.openClass)in this case to be safe.
There was a problem hiding this comment.
Hi @wesleycho,
Thanks for reviewing the PR. I'd like to understand your concern better. Please provide an example so I can get on the same page.
There was a problem hiding this comment.
Any DOM manipulation can be done to change the class of the element - in that case, relying on toggle to correctly toggle the class would be completely incorrect, since it makes a poor assumption that the actual class reflects the state of isOpen. The coupling of the model value and the DOM value is not strong enough to make this assumption in a library.
There was a problem hiding this comment.
@wesleycho
how about
element.toggleClass(scope.openClass, !!value);
is that OK?
There was a problem hiding this comment.
Still has the same flaw of using toggleClass
There was a problem hiding this comment.
@wesleycho
would you provide an example? If you are concerned with toggleClass mashing some other classes, I'm thinking toggleClass plays safe with them just like addClass and removeClass.
There was a problem hiding this comment.
I apologize, I'm on not enough sleep it appears - this is completely fine due to the second parameter. Ignore my complaints here.
There was a problem hiding this comment.
@wesleycho 😁 np. Thanks for staying up and accepting the PR
wesleycho
added
type: enhancement
needs: work
component: accordion
and removed
needs: work
labels
|
This PR LGTM - will merge in sometime today. |
uses toggleClass in isOpen watch
Fixes #4193