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.

Sign up for GitHub

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

Jump to bottom

Fix active class toggling of tabs within dropdown #37151

Merged
merged 2 commits into from
Sep 21, 2022
Merged

Fix active class toggling of tabs within dropdown #37151

merged 2 commits into from
Sep 21, 2022

Conversation

cpsievert
Copy link
Contributor

@cpsievert cpsievert commented Sep 14, 2022 •
edited
Loading

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

@cpsievert cpsievert requested a review from a team as a code owner September 14, 2022 15:52
@cpsievert cpsievert force-pushed the fix-dropdown-active-tab branch from 704d222 to 58be58d Compare September 14, 2022 16:01
@GeoSot GeoSot force-pushed the fix-dropdown-active-tab branch from 3d03976 to 7ff0090 Compare September 21, 2022 07:36
@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 👍

@GeoSot GeoSot force-pushed the fix-dropdown-active-tab branch from 7ff0090 to 7840191 Compare September 21, 2022 23:25
@GeoSot GeoSot merged commit 2b46842 into twbs:main Sep 21, 2022
@GeoSot GeoSot mentioned this pull request Sep 22, 2022
Closed
3 tasks
@cpsievert cpsievert deleted the fix-dropdown-active-tab branch September 22, 2022 14:00
@cpsievert cpsievert mentioned this pull request Sep 23, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GeoSot GeoSot GeoSot approved these changes

Assignees
No one assigned
Labels
js v5
Projects
No open projects
v5.2.2
Status: Done
Milestone
No milestone
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
@cpsievert @GeoSot @julien-deramond