feat(accordion): new accordion directives #4437
Conversation
<div
ngbAccordion
[animation]="animation"
[closeOthers]="closeOthers"
[destroyOnHide]="true"
(shown)="log('shown ' + $event)"
(hidden)="log('hidden ' + $event)"
>
<!-- ngb-accordion-header-XXX-->
<!-- ngb-accordion-collapse-XXX-->
<div ngbAccordionItem [destroyOnHide]="true" [disabled] (shown) (hidden) [collapse]="true">
<h2 ngbAccordionHeader>
<button ngbAccordionToggle>Accordion Item #1</button>
</h2>
<div ngbAccordionCollapse>
<div ngbAccordionBody>
<ng-template>
<strong>This is the first item's accordion body.</strong> It is shown by default, until the collapse plugin
adds the appropriate classes that we use to style each element. These classes control the overall appearance,
as well as the showing and hiding via CSS transitions. You can modify any of this with custom CSS or
overriding our default variables. It's also worth noting that just about any HTML can go within the
<code>.accordion-body</code>, though the transition does limit overflow.
</ng-template>
</div>
</div>
</div>
</div> |
ValentinNelu
force-pushed
the
feat/accordion-directive
branch
3 times, most recently
from
5ea1bbe
to
0c2801b
Compare
|
Need to add proper documentation, and do the demo page. |
ValentinNelu
force-pushed
the
feat/accordion-directive
branch
from
0c2801b
to
fea6361
Compare
Codecov ReportBase: 88.67% // Head: 88.84% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #4437 +/- ##
==========================================
+ Coverage 88.67% 88.84% +0.17%
==========================================
Files 114 115 +1
Lines 3999 4096 +97
Branches 843 864 +21
==========================================
+ Hits 3546 3639 +93
- Misses 413 417 +4
Partials 40 40
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
There was a problem hiding this comment.
Hey, @ValentinNelu, looks good overall!
Left some comments (details in code comments):
- Please use underscore for all private vars
private _varName, notprivate varName. Sorry, it's the repository code style. - I'm not convinces with how
disabledis handled, also the demo doesn't seem to work to me? You should be able to open/ close disabled panels via API, but user shouldn't. Some old tests checking this are missing: ex. we've hadit('should not open disabled panels from click')test andit('should open/collapse disabled panels'). Now you seem to have different behavior.(let's discuss it) - Subscriptions are leaking
- Seems like
destroyOnHidemight not work properly at runtime
I haven't checked test code coverage and API coverage yet.
Will do after your fixes!
Cheers,
Max
src/accordion/accordion.directive.ts
Outdated
| standalone: true, | ||
| imports: [NgTemplateOutlet], | ||
| host: { '[class.accordion-body]': 'true' }, | ||
| template: ` <ng-template [ngTemplateOutlet]="addToDOM() ? bodyTpl : null" ]></ng-template> `, |
There was a problem hiding this comment.
idea: maybe it is even better to extract it into something like
<ng-template [ngTemplateOutlet]="template()"></ng-template>WDYT?
src/accordion/accordion.directive.ts
Outdated
| @ContentChild(TemplateRef) bodyTpl: TemplateRef<any>; | ||
|
|
||
| addToDOM() { | ||
| return this.item.destroyOnHide === false || this.item.animatingBodyCollapse; |
There was a problem hiding this comment.
this.item.destroyOnHide is only initialized once, so it might be wrong I guess.
src/accordion/accordion.directive.ts
Outdated
| export class NgbAccordionBody { | ||
| constructor(@Inject(forwardRef(() => NgbAccordionItem)) private item: NgbAccordionItem) {} | ||
|
|
||
| @ContentChild(TemplateRef) bodyTpl: TemplateRef<any>; |
There was a problem hiding this comment.
Probably should have {static: true} option
| standalone: true, | ||
| selector: '[ngbAccordionCollapse]', | ||
| host: { | ||
| role: 'region', |
There was a problem hiding this comment.
Will a user be able to override a role is necessary?
src/accordion/accordion.directive.ts
Outdated
| '[class.collapsed]': 'item.collapsed', | ||
| '[attr.aria-controls]': 'item.collapseId', | ||
| '[attr.aria-expanded]': '!item.collapsed', | ||
| '[type]': '"button"', |
src/accordion/accordion.directive.ts
Outdated
| * Has no effect if the panel is disabled. | ||
| */ | ||
| toggle(itemId: string) { | ||
| if (this.canBeToggled(itemId)) { |
There was a problem hiding this comment.
doing this.canBeToggled seemed like a lot of redundant work, but indeed I don't see how to do it better...
src/accordion/accordion.directive.ts
Outdated
| } | ||
| } else { | ||
| this.items.forEach((item) => { | ||
| if (!item.disabled) { |
There was a problem hiding this comment.
I'm not sure about disabled handling again... why can't you work with disabled items programmatically? I understand that the user can't do anything with them, but why do APIs not allow it?
src/accordion/accordion.directive.ts
Outdated
| } | ||
|
|
||
| collapse(itemId: string) { | ||
| this.items.find((item) => item.id === itemId)!.collapsed = true; |
There was a problem hiding this comment.
It should fail when find finds no matching item. Tests missing?
src/accordion/accordion.directive.ts
Outdated
| } | ||
|
|
||
| private canBeToggled(itemId: string) { | ||
| return this.items.find((item) => item.id === itemId && !item.disabled) ? true : false; |
There was a problem hiding this comment.
nit: might be simpler with !!this._items.find...
ValentinNelu
force-pushed
the
feat/accordion-directive
branch
from
fea6361
to
1860f55
Compare
ValentinNelu
force-pushed
the
feat/accordion-directive
branch
from
1860f55
to
5a5c86f
Compare
maxokorokov
changed the title
Before submitting a pull request, please make sure you have at least performed the following: