@C-Lodder Can you check if this PR is correct?

@ciar4n Can you check if this PR is correct?

@richard67 I think the idea was not to couple the menu to the bootstrap classes and you're reverting this...

@dgrammatiko So I shall close this PR? Any other idea how I can get a horizontal menu with the Cassiopeia template? Maybe even with nav-pills working?

Let's ask @ciar4n here, I have no clue what the final decisions on templates are...

Ok, am waiting for reply and will of course close this PR if advised to do so.

@dgrammatiko For the navbar-nav class it is right what you say, that it couples the menu to boostrap classes again, so this should maybe be reverted, but I think that removing the flex-column could be a valid change. Maybe @ciar4n can check that, too?

@richard67 I think the idea was not to couple the menu to the bootstrap classes and you're reverting this...

So changing one Bootstrap class (.flex-column) with another (.navbar-nav) means a tighter coupling to Bootstrap? Are we really not going to be allowed to use ANYTHING in the frontend if it has a corresponding selector in the Bootstrap SCSS?

I thought .flex-column was our own class not BS, if not the case then my comments are out of scope

I thought the same thing until this morning. Then I used my Google-fu to confirm ?

@dgrammatiko @mbabker So is this PR right now? PR wrong? Or partly right?

At a glance, not a clue. I ripped most of the BS out of the one BS4 build I have (which incidentally enough doesn't use their nav component at all) ?.

Based on your screenshot it looks right though.

Or, it's purposefully excluded from the menu module because the module can be used in other positions and that class might tie you to use in that position of this particular template's design. I honestly don't know what's going on with the new templates off hand.

@mbabker Then there should be maybe no class at all hard-coded in the module, neither nav nor navbar-nav nor flex-column, and class= is added only if $class_sfx is not empty, i.e. all classes are set with $class_sfx and nothing is hard-coded in default.php?

Without a change in the markup structure I'd leave a nav class, to me that should be mildly generic (unfortunately some frameworks claim and use it).

So have one of these constructs in the default module markup, leave the rest to overrides and UI configuration:

<nav class="<?php echo $class_sfx; ?>">
    <ul>
        <li>Thing</li>
    </li>
</nav>

<ul class="nav <?php echo $class_sfx; ?>">
    <li>Thing</li>
</ul>

(Unfortunately even THAT isn't an easy thing, there isn't a good way for a template to "suggest" classes to add to a module so it renders correctly, meaning you get some quirks if you set up a menu module from scratch and forget to copy the right classes into the config, one of those UX things to think about at some point I suppose)

and dont forget that for a11y it should be wrapped in
<nav role="navigation">

I think this PR is correct in removing the flex-column. I am not so sure about adding navbar-nav as it is more by accident rather than by design that it works in the column styled modules (correct me if I am wrong). Maybe better to apply the row flex-direction in the template CSS for the header module position.

Am not sure if I am the right guy to solve that, since I don't have any idea how it is intended to be and how templates with scss work.
Shall I change this PR so it only removes the flex-column? Or shall I try to find out what to do in scss in addition so the menu looks good in any position? Or shall I close this PR completely and let someone else with more knowledge on all that do it?

Or maybe it needs to create an override for the menu default view in the Cassiopeia template?

@mbabker I should not wrap the ul element into a nav inside modules/mod_menu/tmpl/default.php because there is already a nav element in the Cassiopeia template's index.php which wraps the menu module position.

@brianteeman For the same reason I won't add role="navigation" because the Cassiopeia template's index.php is out of scope of this PR (up to now). And I am not sure if it needs the role="navigation" for a nav element because nav is made for that purpose already, i.e. can be ignored by screen readers per definition.

@ciar4n It seems that the Cassiopeia template needs some work to be done for making horizontal menus and menu classes like nav-pills work. Because you are mentioned as one of the 2 authors of that template I had pinged you here. If you (or anyone else) can give me any idea which way to go, e.g. if the nav element shall stay where it is now in the Cassiopeia template's index.php or if it should be moved to modules/mod_menu/tmpl/default.php, and how the (s)css shall be handled (bootstrap or own styles), then I am willing to do some work and make a better PR. But right now, as long as I have no idea in which direction to go, I will better not do that in order not to mess up things.

Closing in favour of PR #20700 .