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 active class toggling of tabs within dropdown #37151

Merged
merged 2 commits into from Sep 21, 2022

Conversation

cpsievert
Copy link
Contributor

@cpsievert cpsievert commented Sep 14, 2022

Closes #36947 (a regression introduced by #33079, cc @GeoSot)

As the codepen in #36947 demonstrates, when clicking on the 2nd tab in a dropdown, all the tabs within the dropdown are marked with an active class. This happens because, the current logic in _toggleDropdown() toggles the active class on the first tab within the dropdown when it shouldn't be

@GeoSot
Copy link
Member

GeoSot commented Sep 21, 2022

Looking back the commits history,
it seems that Tab was handling only the removal of active class

bootstrap/js/src/tab.js

Lines 116 to 120 in 63d38b1

const dropdownChild = SelectorEngine.findOne(SELECTOR_DROPDOWN_ACTIVE_CHILD, active.parentNode)
if (dropdownChild) {
dropdownChild.classList.remove(CLASS_NAME_ACTIVE)
}

So if we want to bring back the exact old behavior, I think it would be better to rename the const SELECTOR_DROPDOWN_ITEM, to
const SELECTOR_DROPDOWN_ITEM_ACTIVE = '.dropdown-item.active' and use it, instead of remove the handling

@cpsievert what do you think about this?

@cpsievert
Copy link
Contributor Author

@GeoSot I don't think that's necessary since, in the new implementation, by the time we reach _toggleDropdown(), the .active class will already be added/removed from the tab via

bootstrap/js/src/tab.js

Lines 103 to 108 in 37e2e7e

_activate(element, relatedElem) {
if (!element) {
return
}
element.classList.add(CLASS_NAME_ACTIVE)

and

bootstrap/js/src/tab.js

Lines 129 to 134 in 37e2e7e

_deactivate(element, relatedElem) {
if (!element) {
return
}
element.classList.remove(CLASS_NAME_ACTIVE)

@GeoSot
Copy link
Member

GeoSot commented Sep 21, 2022

I 've checked it and you are right. Thanks, @cpsievert 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Tab active class isn't removed correctly when inside drop-down menu (5.2 regression)
3 participants