Skip to content
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 v5 regressions in tab dropdown functionality #33626

Merged
merged 3 commits into from
Apr 15, 2021

Conversation

cpsievert
Copy link
Contributor

@cpsievert cpsievert commented Apr 13, 2021

Refs #33625 (fixes the 1st problem, at least)

I'd also be happy to help address the 2nd issue, but I'm not sure whether you'd prefer to just update the docs, or actually fix the Tab logic to support .dropdown-items wrapped in <li>

<ul class="dropdown-menu">
     <li><a class="dropdown-item" href="#">Action</a></li>
</ul>

To do the latter, looks as though we need to modify the 1st part of this logic to do something closer to const dropdownElement = element.parents(SELECTOR_DROPDOWN).find(CLASS_NAME_DROPDOWN_MENU)

bootstrap/js/src/tab.js

Lines 165 to 174 in db32b23

if (element.parentNode && element.parentNode.classList.contains(CLASS_NAME_DROPDOWN_MENU)) {
const dropdownElement = element.closest(SELECTOR_DROPDOWN)
if (dropdownElement) {
SelectorEngine.find(SELECTOR_DROPDOWN_TOGGLE)
.forEach(dropdown => dropdown.classList.add(CLASS_NAME_ACTIVE))
}
element.setAttribute('aria-expanded', true)
}

@cpsievert cpsievert requested a review from a team as a code owner April 13, 2021 21:32
@cpsievert cpsievert mentioned this pull request Apr 13, 2021
2 tasks
@GeoSot
Copy link
Member

GeoSot commented Apr 14, 2021

@cpsievert can you add a test too?

@GeoSot
Copy link
Member

GeoSot commented Apr 14, 2021

Thank you for you contibution @cpsievert
I would suggest to open another issue/PR for the second case, supporting ul > li > .dropdown-item

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants