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

Dropdown: support .dropdown-item wrapped in <li> tags #33634

Merged
merged 6 commits into from Apr 21, 2021

Conversation

cpsievert
Copy link
Contributor

@cpsievert cpsievert commented Apr 14, 2021

Fully closes #33625 (follow up to #33626)

This adds support for .dropdown-items to be wrapped in <li> tags which is the HTML structure that the docs currently recommend.

It'd be great to backport this fix to BS4 as well since that suffers from the same problem, which is a pretty unfortunate change from BS3 (which required <a> tags to be wrapped in <li>)

@cpsievert cpsievert requested a review from a team as a code owner April 14, 2021 23:16
@XhmikosR XhmikosR changed the title Close #33625: support .dropdown-item wrapped in <li> tags Dropdown: support .dropdown-item wrapped in <li> tags Apr 15, 2021
@XhmikosR XhmikosR added this to Inbox in v5.0.0 via automation Apr 15, 2021
@XhmikosR XhmikosR added this to Inbox in v4.6.1 via automation Apr 15, 2021
@XhmikosR XhmikosR moved this from Inbox to Needs manual backport in v4.6.1 Apr 15, 2021
@XhmikosR
Copy link
Member

@cpsievert feel free to backport it to the v4-dev after this one lands :)

@GeoSot
Copy link
Member

GeoSot commented Apr 15, 2021

@cpsievert please rebase your branch

js/tests/unit/tab.spec.js Outdated Show resolved Hide resolved
cpsievert added a commit to cpsievert/bootstrap that referenced this pull request Apr 15, 2021
@XhmikosR XhmikosR moved this from Inbox to Review in v5.0.0 Apr 18, 2021
v5.0.0 automation moved this from Review to Approved Apr 18, 2021
Copy link
Contributor

@alpadev alpadev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO

bootstrap/js/src/tab.js

Lines 165 to 179 in 25b03ca

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

Could be a bit optimized like

const context = element.closest(`${SELECTOR_DROPDOWN},${SELECTOR_NAV_LIST_GROUP}`) // this is so we're not reaching out of tabs nested in a dropdown (not sure if this could be case)
// if context matches SELECTOR_DROPDOWN we know we are still in the dropdown
if (context && context.matches(SELECTOR_DROPDOWN)) {
  SelectorEngine.find(SELECTOR_DROPDOWN_TOGGLE, context)
    .forEach(dropdown => dropdown.classList.add(CLASS_NAME_ACTIVE))
  element.setAttribute('aria-expanded', true)
}

Also not sure about aria-expanded because this would be added to the dropdown item and that isn't expanded? 🤔

But to keep changes as small as possible, we better go with this one, I guess? @XhmikosR

@XhmikosR
Copy link
Member

Yup, definitely split the changes into separate PRs so that we can track any breakage :)

@XhmikosR
Copy link
Member

@alpadev you want to apply your optimization patch here? Or do you plan to make a PR after this one has landed?

@alpadev
Copy link
Contributor

alpadev commented Apr 20, 2021

@XhmikosR as you prefer. I was thinking to do this after, so we're less likely to introduce more bugs before stable.

@XhmikosR
Copy link
Member

XhmikosR commented Apr 20, 2021 via email

@XhmikosR XhmikosR merged commit 2cbb0a9 into twbs:main Apr 21, 2021
v5.0.0 automation moved this from Approved to Done Apr 21, 2021
@XhmikosR XhmikosR removed this from Needs manual backport in v4.6.1 Apr 21, 2021
@XhmikosR XhmikosR added the js label Apr 22, 2021
XhmikosR pushed a commit that referenced this pull request Apr 28, 2021
Dropdown: support `.dropdown-item` wrapped in `<li>` tags
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v5.0.0
  
Done
Development

Successfully merging this pull request may close these issues.

Issues with Bootstrap 5 dropdown tabs
4 participants