Fix dropdown destroy method

[diniscorreia]

This fixes issue #9339. When destroying a dropdown menu, the dropdown classes wouldn’t be removed if using a different class structure (e.g.:, using custom class names instead of .menu). Previous versions selected all child elements of the menu, but on the latest version only direct li child elements or elements with .menu class are targeted.

This commit ensures ul elements also get the classes removed, for those cases where custom classes are used. Not sure if this is the best way, but seems to cover most cases.

[kball]

Thanks for submitting a PR! You're right that it doesn't necessarily seem like the best way... the reason this was changed to be less broad-reaching was because folks were running into problems with nesting stuff inside menus.

That said, looking at the rest of the Nest code there is already an assumption that uls are going to be submenus as a part of this menu.

Hmm... the one thing I would raise then is that this will only work in the single-nested case, but deeply nested menus will still break. Given we have the ul assumption in Feather, maybe instead of matching against .menu we can match against ul and catch all of our cases?

Would be interested in thoughts from other @zurb/yetinauts on this as well.

[diniscorreia]

@kball So just > li, > li > ul is probably the safest way, no? Even if you add a second level of nesting, this would only target one specific dropdown (.menu on the other hand would destroy all).

[DaSchTour]

That looks really ugly. Shouldn't the selector for finding do a lot less assumptions about the structure of the menu?

[ncoden]

I agree with @DaSchTour, we shouldn't target > li. But unfortunately, this is the current structure of the menu for the CSS.
So or we remain consistant and we can make this ugly assumption in the Js. Or we switch to something like .menu-item.

[brettsmason]

@ncoden I'd personally love to see the menu system redone, as its a bit of a nightmare to overwrite styles at the moment. That and get rid of table-cell, but thats another issue entirely 😉

[ncoden]

@brettsmason I remember that on my previous project, I preferred reimplement a menu by myself rather than using the Foundation menu and overwriting everything. 😆

[diniscorreia]

@ncoden relying on class names seems more problematic.

I stumbled upon the problem because I'm building my own menu structure and class names using Foundation's mixins. So targeting a basic menu structure (lists) or just going back the previous way (selecting *) seems less prone to errors.

[ncoden]

@diniscorreia I didn't mean that .menu-item would be used in the JS, but only to apply CSS styles (the styles we want, where we want it, when we want it). That's a bad practice to target classes or tags (<li>) in the Js. We should use [data-*] attributes instead.

I thought about it, but I really don't know. I don't think I seen data attributes used for sub-elements before: (poke @kball)

<ul data-menu class="menu">
    <li data-menu-item class="menu-item">Item 1</li>
    <li data-menu-item class="menu-item">Item 2</li>
    ...
</ul>

Note: If we care about perf issues, we can use .js-* classes the same way

[DaSchTour]

@ncoden from what I have heard recently the performance gap between class and attribute selector isn't that big and that there are a lot of operations that decrease performance a lot more.
I also don't like the idea of adding .js-* classes while all other components use data attributes.

[ncoden]

@DaSchTour I was talking generally. Anyway, the performance gap between classes and data attributes is for the browsers that don't support querySelector, so IE <= 8.

My main concern is about using data attributes for the sub-elements of a component. 🤔

[kball]

This gets to a really tricky line around how verbose we want to force the markup to be vs not, and it is something we're going to need to figure out moving forward...

We have a couple of examples where we use data attributes on sub items before (magellan targets, equalizer watches)... We could start standardizing that.

Another thing that we could consider (not sure if this is a good idea or not) is making it configurable via a javascript flag... e.g. for folks using the "default" approach we auto-apply the data attributes for children... this lets us have the 'base case' be still minimal markup. But if you want to do more complicated things or get more control, you set a 'data-manual-children' attribute on the parent and then you mark it up... making it easier to explicitly control component nesting etc.

[ncoden]

@kball 👍 totally agree

[ncoden]

@kball @DaSchTour @brettsmason So, what do we do for this PR ?

[ncoden]

LGTM 👍
I don't like having structure assomptions in the selector but (see the comments above) this is the way menu works.