fix(accordion): ids that begin with number were not accepted, #4195 #4196
Conversation
ExFlo
force-pushed
the
fix/accordionsId
branch
8 times, most recently
from
ed0ea61
to
927daf2
Compare
ExFlo
force-pushed
the
fix/accordionsId
branch
from
927daf2
to
9b4a23b
Compare
Codecov Report
@@ Coverage Diff @@
## master #4196 +/- ##
==========================================
- Coverage 88.48% 88.46% -0.03%
==========================================
Files 110 111 +1
Lines 3283 3623 +340
Branches 639 690 +51
==========================================
+ Hits 2905 3205 +300
- Misses 325 368 +43
+ Partials 53 50 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Hey,
is there actually an issue for this?
The one that is referenced is about something else.
My bad, it's fine
I'm not convinced by passing the id as the event on init and having a special directive for this on my side :)
Can't we just change the query? WDYT?
| @@ -425,10 +436,6 @@ export class NgbAccordion implements AfterContentChecked { | |||
| } | |||
| }); | |||
| } | |||
|
|
|||
| private _getPanelElement(panelId: string): HTMLElement | null { | |||
| return this._element.nativeElement.querySelector('#' + panelId); | |||
There was a problem hiding this comment.
Can't we just querySelector('[id="{panelId}"]') ?
| @@ -195,7 +208,7 @@ export interface NgbPanelChangeEvent { | |||
| <ng-template [ngTemplateOutlet]="panel.headerTpl?.templateRef || t" | |||
| [ngTemplateOutletContext]="{$implicit: panel, opened: panel.isOpen}"></ng-template> | |||
| </div> | |||
| <div id="{{panel.id}}" role="tabpanel" [attr.aria-labelledby]="panel.id + '-header'" | |||
| <div id="{{panel.id}}" (ngbRef)="panel.panelDiv = $event" role="tabpanel" [attr.aria-labelledby]="panel.id + '-header'" | |||
There was a problem hiding this comment.
Oh, Im so not convinced by passing id as the event on init :)
There was a problem hiding this comment.
Note that this is not an id, it is the reference to the element itself, thus removing the need of an id.
There was a problem hiding this comment.
Sure. But on the other hand I just thought that it also avoids doing querySelector for each panel on every ngAfterContentChecked...
There was a problem hiding this comment.
ngbRef seems to be inspired from callback refs in react.
We could instead use refs object as in react which would perhaps convince you more than the callback form.
There was a problem hiding this comment.
I see, and on my side I can't really find a simpler way to pass the element ref once during the initialization
There was a problem hiding this comment.
Hmm, on reflection:
I don't like the idea of firing events in lifecycle hooks, it sometimes leads to nasty side-effects, doesn't it? However in this case ids never change and it's pretty simple, so it looks ok to me. I can't find any simpler ways to do it once on my side.
Simpler version would be to just change selector to use the attribute, but this PR also removes doing querySelector on each ngAfterContentChecked which is a good thing.
I think it is LGTM.
fix #4195