Replace the "Primary menu" component with a reusable component in Bartik.

Thanks for championing this @emma.maria

Can this be done in 8.0?

This patch appears to work but please test. Since the class name menu--main uses the menu's machine name drupal guarantees that it is unique and so it is a good selector. But please note that since the region css components contain so much styling that this and the secondary menu will look different in each region it is placed, which is probably what should happen. And the corollary is that any block moved to these regions will also look differently than in their original placement.

Instead of overwriting whole template why not just simply:

{% extends "@classy/layout/region.html.twig" %}
{% set attributes = attributes.addClass('primary-menu', 'layout-' ~ region|clean_class) %}

Updated the region--primary-menu.html.twig as requested in #12

This patch implements the suggestion in #11 and breaks the toggle code into a separate file. I'm not quite sure what the boundaries are there though. It needs more work. For example, is the assumption that block labels are invisible in these menu areas true?

@emma.maria Uploading the re-rolled patch for #14. Attaching screenshot too.

Above patch committed the changes for the new files. Updated and uploading. Interdiff attached.

My internet really went crazy and created all the mess above. Sorry for all that. Please ignore comments from #18 - #20.

This #21 has all the final patch re-roll updates + screenshot.

This patch is just to reset the patch. It goes back to when the toggle code was part of primary-menu.css and adds a @file statement to it. The code seems to very fragile, as simply moving hunks of code around breaks the layout pretty badly.

This is a small fix because of a visual regression introduced by this patch. The css here seems very fragile.

I just started to check this issue and wonder why we have the name of the primary menu region from its default content? It should have a more generic name maybe just as the Messages region was renamed to Highlighted.

See #2513526: Rename the menu regions

Thanks @dcrocks for pointing the related issue.

Fix misleading part of issue description possibly coming from the creating of the issue by cloning an other one.

About the last patch in #28

• I was not able to find any visual regression
• If I understand well, @emma.maria asks in #25 to move the related media queries next to the affected selectors instead of creating only one block for every media query. This is missing.
• According to our CSS coding standards media query values should be defined using rems.
• According to Can I Use the :target selector is supported by IE9 (as well as all other current browsers) so body:not(:target) selectors should be removed from the code (see also #25).

1. +++ b/core/themes/bartik/css/components/primary-menu.css
   @@ -110,13 +110,13 @@ body:not(:target) .region-primary-menu .menu-toggle-target-show:target ~ .menu .
        margin: 0 5px;
        padding: 0;
   

   Redundant, already defined at line 8–9

2. +++ b/core/themes/bartik/css/components/primary-menu.css
   @@ -110,13 +110,13 @@ body:not(:target) .region-primary-menu .menu-toggle-target-show:target ~ .menu .
        padding: 0;
   
   @@ -125,39 +125,39 @@ body:not(:target) .region-primary-menu .menu-toggle-target-show:target ~ .menu .
        height: auto;
   

   Redundant. Already defined at line 19-20.

3. +++ b/core/themes/bartik/css/components/primary-menu.css
   @@ -125,39 +125,39 @@ body:not(:target) .region-primary-menu .menu-toggle-target-show:target ~ .menu .
        font-size: 0.929em;
   

   Redundant. Already defined at line 7.

There could be more redundant code like these.

Also the declaration order in the rulesets should be set aligned to our standards.

Otherwise I am totally confused by this issue.

The title says we want to

Replace the "Primary menu" component with a reusable component in Bartik

However Primary menu is a region. If I move its html code to somewhere else its content will not look like the same (using the current patch).

Later @emma.maria writes in #6 that

We want any menu added to this region to have the "main navigation" styles.

and

Therefore we need to add a reusable component class to menu blocks when they are added to the currently named "Primary menu" region

However what the current patch do is adding a new class .primary-menu to the region and using that class to style menus instead the original .region-primary-menu class. I can't see the difference why it is better?

If we want to to style menu block when they are in this specific region, the current (not patched) solution just works.

If we want to be able to style a manu to look like the current main menu in the primary region independently from its position than we should create a menu--primary modifier for the menu component and make it possible to apply that class to menu blocks when it is needed.

A good question. The fact is, for bartik, menu styling is different for different regions. So yes, the menu styling is tied to the region but that styling shouldn't be tied to the region name, as other content might go into a given region. So this patch provides a region twig file to identify the menu styling for this region. That file's name will change when the region does get renamed but the menu classes added wont.

Just a note, removing all the 'body:not(:target) selectors' doesn't seem to introduce any visual regression on firefox at least and also removed the redundant code mentioned in #34. I'll provide a new patch soon.

This removes all the 'body:not(:target)' selectors. However just moving multimedia code around introduces visual regressions. That will take more thought.

The last is not going to work.

This design is mobile 1st. Removing toggle code causes visual regressions. Dispersing media code causes visual regressions. I would like to go with #28 and if someone thinks serious css improvements need to made then that could possibly be a follow up issue.

Removed all 'body:not(:target)' selectors. This time did it correctly. Tried in firefox, chrome, safari, and opera. Appears to work OK.

This disperses the multimedia code thru-out the file.

@dcrocks Thanks the info and the patches. I try to check them (if I have enough time)

About the issue: so the idea is that the region name will change but it will keep the currently introduced .primary-menu class? If so that will be confusing. It basically not the best idea to modify the styling of a component (menu) because of its position in the layout (.region-name .component name {sepcific: styling;}) – but if we do so, it is much better to use the name of the layout element then introduce a new class which is hard to identify. (The recommended and reusable way would be to add a modifier to the component: .menu--primary.) I just not understand how will .primary-menu be reusable. Also how other elements than menus in this region will be styled? Will they have an other class?

Sorry if these all clear to others. I try to understand it…

There is a lot of history there. This is my understanding of it. The bartik theme, and the Gardner theme before it, always had 2 menus in the header area. The variable names used to place them were 'primary-menu' and 'secondary-menu'. These menus are identical in output to the 'User account' and 'Main navigation' menus but they have a special use case in which they could be joined to make one multilevel menu. I've never used that option but apparently a lot of others have.
In drupal 8, these menus were converted to blocks while still preserving the special use case. That was part of the effort to eliminate system variables in templates, which you can see in many other issues. In the process of creating these blocks the header area was broken into 3 regions with the current names.
But those names don't meet standards so there is an effort to rename them. And it is also a standard for any menu placed in a given region to have a styling appropriate to that region's over all design requirements. For the header area, in what are now called the primary- and secondary- menu regions, the styling for any menu has to match the historical primary and secondary menu styling, upgraded to support mobile devices.
The purpose of this issue is to free that styling from the name of the region while still maintaining the historical connection to the primary and secondary menus. When the issue is complete, any menu placed in this region will have the same styling. It is also intended as part of a general cleanup that is being down on all core css.
I hope this helps. I hope there isn't any major error in fact or interpretation, but hopefully others will correct me.

I do agree that the term 'reusable component' is confusing here. We are really providing standardized styling for one particular 'type' of block in this specific region.

Here is an example of the Tools menu added to the primary menu region.

@dcrocks Thanks for not giving up explaining the issue for me!

@dcrocks Thanks for your patch! While I still have concerns about the issue I used your patch as a base for refactoring and cleaning up more the file. The result of our effort is 175 lines instead of 193 and 3.7 KB instead of 4.7 KB. :)

So it still needs review and the one who review it please read our discussion about the goal and the solution of this issue.

Did a new git clone of 8.0.x and applied patch from #47. Installed cleanly and saw no visual regressions. Did notice that both the 'Place block' and 'Configure block' form allow you to toggle the 'Display title' option, which means you can change that option on both new menu placements as well as existing ones. Takes care of my issue with that feature.

@thamas I know you have other things to do, but could you take a look at the sister issue, #2502049: Replace the "Secondary menu" component with a reusable component in Bartik. if you have time?

@dcrocks I can't promise it very soon, but I'll check that.

Back to this issue I wonder why we do not have .menu__link classes because styling html elements (in this case the a element) is not a good practice…

Struggling to find generic way to add class menu__link to 'a' elements of components of type primary-menu. Complicated by fact that a special class 'is-active' is added to all menus but by different means if anonymous or logged in users(php vs javascript). So far, can't prevent visual regressions. Added screenshots.

I don't think etiquette allows me to mark this RTBC but I think #47 meets all the requirements specified in the issue summary and in discussion. As far as providing a class name for 'a' elements, the only way to properly solve it is as a generic issue for all drupal css, not just for the primary menu.

While I think this issue should be RTBC here is some addition to my toughts in #42:

Because of violation of the Open/Closed Principle, we have ended up with a dictatorial selector. We have written a selector that says "If you put that in here, this will happen." This means we are now open to leaking styles: because the decision is made in our CSS and not in our view, things will happen whether we like them or not.

Harry Roberts, in Contextual Styling: UI Components, Nesting, and Implementation Detail (see the Implementation detail part for, ehm… details :)).

Rerolled this for current 8.1.

Did that wrong. This is a patch against current 8.0.

Hi,

The patch of #55 is not working here is the screen short which can be help to find the error.

Thanks!

Hi @djsagar, looks like you applied the interdiff text file instead of the patch file...

Here's the re-rolled the patch in #55 against the 9.1.x dev branch.

Thanks!

Thanks to everyone for the patch. It's applied successfully.

Here's the re-rolled the patch in #65 against the 9.3.x

Thanks!

I have attached a patch and interdiff as well. Please review.

Hi @Gauravmahlawat thanks for the patch, here am re-rolling the patch by resolving the custom failures, please review the patch

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge require as a guide.

Reviewing the last patch this appears to be bartik specific. Bartik was removed in Drupal 10 so moving this ticket to the contrib module