Dropdown: support .dropdown-item wrapped in <li> tags#33634
Dropdown: support .dropdown-item wrapped in <li> tags#33634
.dropdown-item wrapped in <li> tags#33634Conversation
.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.
IMHO
Lines 165 to 179 in 25b03ca
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-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>)