fix(accordion): do not use static header level for heading #2915
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2915 +/- ##
==========================================
- Coverage 91.23% 91.21% -0.02%
==========================================
Files 89 89
Lines 2885 2879 -6
Branches 475 472 -3
==========================================
- Hits 2632 2626 -6
Misses 181 181
Partials 72 72
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Hey @peterblazejewicz, thanks for the PR!
I've just read WAI-ARIA documentation and opened #2927 to address differences.
I don't think just adding role=heading on top of role=tab is a good solution.
So if you don't mind let's transform this to the fix(accordion) and
- replace
<h5>→<div>in this PR as discussed in Feature Request: ngb-accordion card-header accessibility #2847 - find out why we need this intermediate element at all (I don't think we actually need it)
Cheers
peterblazejewicz
force-pushed
the
feat/2847
branch
from
eae6922
to
2c42d92
Compare
peterblazejewicz
changed the title
|
@maxokorokov |
src/accordion/accordion.ts
Outdated
| @@ -115,13 +115,13 @@ export interface NgbPanelChangeEvent { | |||
| <ng-template ngFor let-panel [ngForOf]="panels"> | |||
| <div class="card"> | |||
| <div role="tab" id="{{panel.id}}-header" [class]="'card-header ' + (panel.type ? 'bg-'+panel.type: type ? 'bg-'+type : '')"> | |||
| <h5 class="mb-0"> | |||
| <div> | |||
There was a problem hiding this comment.
Don't you think that in this case we can remove this element altogether? It doesn't affect rendering, so I don't see any reasons in having it at all (that was the second part of my previous review).
On the bootstrap side they've changed example from <h5> → <h2>, but again it's just an example. Any kind of markup could be put into cards.
There was a problem hiding this comment.
agree, removed the wrapping div completely in the updated version.
Thanks!
- replace h5 header tag with div tag Thanks!
peterblazejewicz
force-pushed
the
feat/2847
branch
from
2c42d92
to
5ddacc0
Compare
|
LGTM, thanks! |
|
After updating to 4.0.2, I'm still seeing the header text running out of the container. I thought it may be my code, so I tested on the StackBlitz on the components/accordion/examples page. It is still running over rather than word-wrapping there too. Am I missing something? |
|
@troddick
This is the BS version used in your SBlitz |
|
That did it! I didn't realize I needed to update BS also. It's working for me now. |
As discussed in #2847, this PR:
h5header tag withdivtag removing no longer necessarymb-0helper class- add heading role attribute to thedivtaghttps://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/heading_role
Thanks!