-
-
Notifications
You must be signed in to change notification settings - Fork 78.9k
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
Conversation
.dropdown-item
wrapped in <li>
tags
@cpsievert feel free to backport it to the v4-dev after this one lands :) |
@cpsievert please rebase your branch |
6cd2270
to
25b03ca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO
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
Yup, definitely split the changes into separate PRs so that we can track any breakage :) |
@alpadev you want to apply your optimization patch here? Or do you plan to make a PR after this one has landed? |
@XhmikosR as you prefer. I was thinking to do this after, so we're less likely to introduce more bugs before stable. |
Alright yeah let's do it separately to be safe
…On Tue, Apr 20, 2021, 16:16 alpadev ***@***.***> wrote:
@XhmikosR <https://github.com/XhmikosR> as you prefer. I was thinking to
do this after, so we're unlikely to introduce more bugs before stable.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#33634 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACVLNI625VTCI6QODOYDN3TJV5CBANCNFSM426KH6AQ>
.
|
Fully closes #33625 (follow up to #33626)
This adds support for
.dropdown-item
s 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>
)