feat(accordion): support nodes preservation #1370
Conversation
src/accordion/accordion.ts
Outdated
| @@ -199,6 +207,14 @@ export class NgbAccordion implements AfterContentChecked { | |||
| */ | |||
| isOpen(panelId: string): boolean { return this._states.get(panelId); } | |||
|
|
|||
| /** | |||
| * @internal | |||
There was a problem hiding this comment.
Using @internal will make it fail in the AoT mode, please remove. I think that it would be better to just in-line this logic in a template and IMO we should just remove this method altogether.
There was a problem hiding this comment.
Are you sure about @internal? I believe AOT has problems with private methods/fields used in the template. Note then that isOpen also has @internal comment.
About inlining the logic I completely agree but there's currenctly a strange behaviour where null is converted into false and the attribute is the added hidden="false". It's then nevertheless selected by Bootstrap CSS selector [hidden] {...}.
I found a similar issue.
There was a problem hiding this comment.
Was pretty sure about the @internal but then noticed that it is used on isOpen... Need to investigate.
In any case let's try to in-line this method (and yes, you need to return null if you want to remove the attribute altogether).
There was a problem hiding this comment.
In any case let's try to in-line this method (and yes, you need to return null if you want to remove the attribute altogether).
The problem is that, with inline null, it becomes false and the attribute is not removed from DOM, it's added as hidden="false" instead. That's the reason of a method. Personally I don't understand why result is different from methods vs inline.
There was a problem hiding this comment.
@jiayihu once again, the main problem with the hidden attribute is that ... you shouldn't use it at all!!! Please use appropriate Bootstrap 4 CSS classes instead, following Bootstrap's markup as documented officially: https://v4-alpha.getbootstrap.com/components/collapse/#accordion-example
BTW, we might need to merge #1332 first.
There was a problem hiding this comment.
In this case I left hidden because there are no examples of accordion component in Bootstrap 4 examples for Card and active would require writing custom CSS to hide the panel
I didn't find the example :P Anyway I'll make the proper changes then, thanks for pointing the example.
|
@jiayihu thnx - it looks good, generally speaking but there is one change I need you to do before we can merge this one - please see comments in the code. |
|
@pkozlowski-opensource I replaced |
pkozlowski-opensource
added
needs: review
and removed
blocked: Bootstrap CSS issue
needs: work
labels
|
Merged, thnx! Sorry it took so long, been busy with preparing 1.0 release. Once again, thnx, appreciate all the PRs! |
Before submitting a pull request, please make sure you have at least performed the following:
Fixes #1161 and #1167
In this case I left
hiddenbecause there are no examples ofaccordioncomponent in Bootstrap 4 examples forCardandactivewould require writing custom CSS to hide the panel