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

Commit

Permalink
fix(accordion): correct is-open handling for dynamic groups
Browse files Browse the repository at this point in the history 
Closes #1297
  • Loading branch information
@bekos @pkozlowski-opensource
bekos authored and pkozlowski-opensource committed Nov 26, 2013
1 parent 2abadca commit 9ec2128
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 13 deletions.
7 changes: 2 additions & 5 deletions src/accordion/accordion.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,6 @@ angular.module('ui.bootstrap.accordion', ['ui.bootstrap.collapse'])
// This array keeps track of the accordion groups
this.groups = [];

// Keep reference to user's scope to properly assign `is-open`
this.scope = $scope;

// Ensure that all the groups in this accordion are closed, unless close-others explicitly says not to
this.closeOthers = function(openGroup) {
var closeOthers = angular.isDefined($attrs.closeOthers) ? $scope.$eval($attrs.closeOthers) : accordionConfig.closeOthers;
Expand Down Expand Up @@ -81,7 +78,7 @@ angular.module('ui.bootstrap.accordion', ['ui.bootstrap.collapse'])
getIsOpen = $parse(attrs.isOpen);
setIsOpen = getIsOpen.assign;

accordionCtrl.scope.$watch(getIsOpen, function(value) {
scope.$parent.$watch(getIsOpen, function(value) {
scope.isOpen = !!value;
});
}
Expand All @@ -91,7 +88,7 @@ angular.module('ui.bootstrap.accordion', ['ui.bootstrap.collapse'])
accordionCtrl.closeOthers(scope);
}
if ( setIsOpen ) {
setIsOpen(accordionCtrl.scope, value);
setIsOpen(scope.$parent, value);
}
});
}
Expand Down
51 changes: 43 additions & 8 deletions src/accordion/test/accordion.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -217,13 +217,12 @@ describe('accordion', function () {
describe('is-open attribute', function() {
beforeEach(function () {
var tpl =
"<accordion>" +
"<accordion-group heading=\"title 1\" is-open=\"open1\">Content 1</accordion-group>" +
"<accordion-group heading=\"title 2\" is-open=\"open2\">Content 2</accordion-group>" +
"</accordion>";
'<accordion>' +
'<accordion-group heading="title 1" is-open="open.first">Content 1</accordion-group>' +
'<accordion-group heading="title 2" is-open="open.second">Content 2</accordion-group>' +

This comment has been minimized.

Sign in to view

Copy link
@chrisirhc

chrisirhc Dec 13, 2013

Contributor

This is definitely the way to go. isOpen is a model and must have a dot in the expression. 👍
'</accordion>';
element = angular.element(tpl);
scope.open1 = false;
scope.open2 = true;
scope.open = { first: false, second: true };
$compile(element)(scope);
scope.$digest();
groups = element.find('.accordion-group');
Expand All @@ -237,11 +236,11 @@ describe('accordion', function () {
it('should toggle variable on element click', function() {
findGroupLink(0).click();
scope.$digest();
expect(scope.open1).toBe(true);
expect(scope.open.first).toBe(true);

findGroupLink(0).click();
scope.$digest();
expect(scope.open1).toBe(false);
expect(scope.open.second).toBe(false);
});
});

Expand Down Expand Up @@ -272,6 +271,42 @@ describe('accordion', function () {
});
});

describe('is-open attribute with dynamic groups', function () {
var model;
beforeEach(function () {
var tpl =
'<accordion>' +
'<accordion-group ng-repeat="group in groups" heading="{{group.name}}" is-open="group.open">{{group.content}}</accordion-group>' +
'</accordion>';
element = angular.element(tpl);
scope.groups = [
{name: 'title 1', content: 'Content 1', open: false},
{name: 'title 2', content: 'Content 2', open: true}
];
$compile(element)(scope);
scope.$digest();

groups = element.find('.accordion-group');
});

it('should have visible group body when the group with isOpen set to true', function () {
expect(findGroupBody(0).scope().isOpen).toBe(false);
expect(findGroupBody(1).scope().isOpen).toBe(true);
});

it('should toggle element on click', function() {
findGroupLink(0).click();
scope.$digest();
expect(findGroupBody(0).scope().isOpen).toBe(true);
expect(scope.groups[0].open).toBe(true);

findGroupLink(0).click();
scope.$digest();
expect(findGroupBody(0).scope().isOpen).toBe(false);
expect(scope.groups[0].open).toBe(false);
});
});

describe('accordion-heading element', function() {
beforeEach(function() {
var tpl =
Expand Down

4 comments on commit 9ec2128

@Sanaliekki
Copy link

Choose a reason for hiding this comment

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

I made these changes to my js file, but it still didn't work. Is it even supposed to work, or are there also some other changes elsewhere that don't show here? I spent a couple of hours trying to create a plunker but failed miserably and gave up.

My arrangement is such that there is a "tabset" with a few tabs created dynamically with an "ng-repeat". On one of the tabs I do something that broadcasts an event in $rootScope, which is caught by a controller for another tab with the accordions on it. Depending on the event parameters one or more accordions should open up but nothing happens even after including the fix shown up there.

And after this when I click the accordion that should have opened, it seems to open up briefly just to close again immediately. Clearly the component's state is "open" but the UI thinks otherwise.

When I change to version 0.6.0 it works fine and it's good enough for me for now. I'm simply concerned that 0.8.0 will still be broken for my project.

@bekos
Copy link
Contributor Author

@bekos bekos commented on 9ec2128 Dec 19, 2013

Choose a reason for hiding this comment

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

@Sanaliekki Please provide a minimal reproduce scenario, otherwise we cannot understand the actual problem.

@Sanaliekki
Copy link

Choose a reason for hiding this comment

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

@bekos Sorry, I tried with plunker but being a n00b I couldn't make it work at all and can't spare enough time right now to try again.. Maybe later.

But it's okay, at least it's working with 0.6.0. I guess the problem could very well be on this end. I just thought I'd try to describe my conundrum in case there's something you missed with the fix... like... do the accordions (and accordion groups) work under tabs that are created dynamically with "ng-repeat"?

I'm still having problems with initializing "ng-grids" in a tab that is inactive. They don't show at all unless I manually rebuild the grid... and I thought this could be something similar.

Thanks!

@Sanaliekki
Copy link

Choose a reason for hiding this comment

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

@bekos Hi, I was finally able to make the plunker work: http://plnkr.co/edit/eQiEspAI3DTLHptHr81f?p=preview

Trying to use the "Open..." buttons leaves the accordions somehow in an intermediate state.
The checkboxes on the second pane are bound to the accordions' "is-open" property so you can see if the accordion should be open or closed.
And obviously you can manipulate the "is-open" property also by clicking the checkboxes, which works perfectly.

Please sign in to comment.