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
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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issues with Bootstrap 5 dropdown tabs
4 participants