Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

fix(accordion): add attr.openClass || 'panel-open' class when expanded #4198

Closed
wants to merge 1 commit into from
Closed

fix(accordion): add attr.openClass || 'panel-open' class when expanded #4198

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/accordion/accordion.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,9 @@ angular.module('ui.bootstrap.accordion', ['ui.bootstrap.collapse'])
link: function(scope, element, attrs, accordionCtrl) {
accordionCtrl.addGroup(scope);

scope.openClass = attrs.openClass || 'panel-open';
scope.$watch('isOpen', function(value) {
element.toggleClass(scope.openClass, value);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wesleycho
how about
element.toggleClass(scope.openClass, !!value);
is that OK?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still has the same flaw of using toggleClass

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I apologize, I'm on not enough sleep it appears - this is completely fine due to the second parameter. Ignore my complaints here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wesleycho 😁 np. Thanks for staying up and accepting the PR

if (value) {
accordionCtrl.closeOthers(scope);
}
Expand Down
30 changes: 29 additions & 1 deletion src/accordion/test/accordion.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ describe('accordion', function() {
expect(findGroupBody(0).scope().isOpen).toBe(false);
});

it('should add "open" when opened', function() {
it('should add, by default, "panel-open" when opened', function() {
var group = groups.eq(0);
findGroupLink(0).click();
scope.$digest();
Expand All @@ -224,6 +224,34 @@ describe('accordion', function() {
});
});

describe('with open-class attribute', function() {
beforeEach(function() {
var tpl =
'<accordion>' +
'<accordion-group heading="title 1" open-class="custom-open-class">Content 1</accordion-group>' +
'<accordion-group heading="title 2" open-class="custom-open-class">Content 2</accordion-group>' +
'</accordion>';
element = angular.element(tpl);
$compile(element)(scope);
scope.$digest();
groups = element.find('.panel');
});
afterEach(function() {
element.remove();
});

it('should add &open-class when opened', function() {
var group = groups.eq(0);
findGroupLink(0).click();
scope.$digest();
expect(group).toHaveClass('custom-open-class');

findGroupLink(0).click();
scope.$digest();
expect(group).not.toHaveClass('custom-open-class');
});
});

describe('with dynamic panels', function() {
var model;
beforeEach(function() {
Expand Down
2 changes: 1 addition & 1 deletion template/accordion/accordion-group.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<div class="panel panel-default" ng-class="{'panel-open': isOpen}">
<div class="panel panel-default">
<div class="panel-heading">
<h4 class="panel-title">
<a href tabindex="0" class="accordion-toggle" ng-click="toggleOpen()" accordion-transclude="heading"><span ng-class="{'text-muted': isDisabled}">{{heading}}</span></a>
Expand Down