-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(accordion): collapsed class not correct #2555
Conversation
src/accordion/accordion.spec.ts
Outdated
@@ -322,6 +322,21 @@ describe('ngb-accordion', () => { | |||
}); | |||
}); | |||
|
|||
it('should have the appropriate CSS collapsed classes', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't we add assert to https://github.com/ExFlo/ng-boostrap/blob/e1064feca33b001b9cad323edb4f5ffbb0c0373a/src/accordion/accordion.spec.ts#L37 instead of writing whole new test?
The advantage is that it would be verified with all the existing tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can but we are not going to test that the class is present in case of close panels just that it is not present in case of open ones. Do we want only this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ExFlo I was more thinking of sth like:
panelsTitles.map((titleEl: HTMLButtonElement) => {
const isAriaExpanded = titleEl.getAttribute('aria-expanded') === 'true';
const isCSSCollapsed = titleEl.classList.contains('collapsed');
return isAriaExpanded === !isCSSCollapsed ? isAriaExpanded : fail('inconsistent state');
});
not well thought-through, just a rough idea...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we just test that collapsed is correctly removed, but not that it is in place the first time right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
During the first time rendering we should have:
isAriaExpanded=false
isCSSCollapsed=true
Given this the isAriaExpanded === !isCSSCollapsed
condition should be true
and the return value false
.
If the class wouldn't be here initially we would get:
isAriaExpanded=false
isCSSCollapsed=false
and the isAriaExpanded === !isCSSCollapsed
test would fail breaking the test. So I believe that the case of initial display would be covered as well.
But hey, this is just a rough idea, feel free to adapt / discard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right I missed the use case of the function. Thx
e1064fe
to
cfa6176
Compare
fixes #2553
Before submitting a pull request, please make sure you have at least performed the following: