Bartik - Responsive menu-toggle shows up in show mode after switching from mobile to pad or desktop display

test it

Tested this in Chrome and firefox it was working good as specified, Test is done using developer toolbar.

Steps done for testing

1. Applied patch
2. resized for mobile -> show navigation visible -> clicked on show navigation -> menu item visible
3. resized for pad -> hide navigation was hidden
4. resized for desktop -> menu item visible normally

Thanks, assigning to @emma.maria to take a look. This could use screenshots as well.

Attaching screenshots

The more pressing bug that I see here is that the text...

"Show - Main Navigation"
"Hide - Main Navigation"

... is always visible on the mobile menu bar.

This was changed in #2547639: Rename ambiguous "Menu" toggle in Bartik for accessibility, initially because of this:
2 menus in mobile display

Oh! I was completely unaware of this issue. I'm normally flagged for Bartik changes.

Thank you @dcrocks!!!

Code review of #2.

Instead of repeating the styles within two separate media queries (tablet width and then the desktop width), we should just add a tablet and up media query for this code.

We are only targeting the mobile toggle, which has no further state changes above 461px.

For eg.

@media all and (min-width: 461px) {
  body:not(:target) .region-primary-menu .menu-toggle-target-show:target ~ .menu-toggle--hide {
    display: none;
  }
}

Hi @meichr thanks for the patch.

Ignoring how the rest of the file is currently structured (it is in the process of being refactored here - #2502045: Replace the "Primary menu" component with a reusable component in Bartik.) please can you make the following amendments to the patch to help it pass Drupal CSS coding standards.

Can the comment be placed completely outside of the ruleset and be placed in a Doxygen comment style. The media query is helping to set a state onto the component (ie. the component is hidden when the screen are these dimensions) and we want the comment to explain the whole scenario of the code we are adding here.

See the coding standards for how to comment for a complete ruleset here:
https://www.drupal.org/node/1887862#multi-line-comments

Also to keep things succinct, the comment copy can be shortened to something like:
"Ensures that the open mobile menu hides when the screen dimensions become bigger than 461px."

If you feel it needs more to explain why we adding this style you can also add...
"- see *insert URL to this issue*"
at the end of the comment.

Thanks!

Hi @meichr

We want to try and move away from everything being grouped by media queries. You lose the ease of seeing how a component is styled at different states if it's scattered throughout a file. Also not every component needs styles for every media query and it definitely becomes harder to maintain if you need to jump around the file looking for all the styles for one component.

We ideally want to have all of the code for a component grouped together in a file with each media query it uses.

For example.

.open-menu {
  *styles*
}
@media all and (min-width: 300px) {
  .open-menu {
      *styles*
  }
}
@media all and (min-width: 600px) {
  .open-menu {
      *styles*
  }
}

So for the question....

Would there ever be another ruleset to be added to this same media query the existing comment then would have to be placed inside the media query before the hiding ruleset.

If another component needs the same media query, we would add another media query declaration around that other component. Yes there will be duplicate media queries all over the file, but the organisation of the styles should be component first and not media query first.

Also answering...

Regarding #2502045: Replace the "Primary menu" component with a reusable component in Bartik.: should this patch be worked into the current patch over there? Or do you plan to commit this patch individually to 8.1.x?

Keep this work only in this issue. This bug issue will be committed before that bigger task and therefore this work will automatically end up in that issue from it being applied to the core codebase.

Thanks for the patch @meichr!

The last step is for someone apart from @meichr to add some review screenshots (showing the menu is now hidden at tablet and desktop widths) and set it to RTBC. I will do this later today if no one else does.

I'll do the screenshots

I followed steps in issue summary and things are resolved with patch from #22.

Thanks @Pnoi for the review!

Suggested commit message:

git commit -m 'Issue #2661128 by meichr, kannan@kiluvai.com, Pnoi, emma.maria: Bartik - Responsive menu-toggle shows up in show mode after switching from mobile to pad or desktop display'

Since this involves a visual bugfix to Bartik, tagging for RC target triage.

Discussed with @xjm and we agreed that this is not a high impact bugfix (kind of an edge case and not really that broken/disruptive to users IMO even when the bug does happen), so removing from RC triage and moving to 8.2.x. https://www.drupal.org/core/d8-allowed-changes#rc

This took me a while to decode the issue summary and reproduce so I've updated it slightly, mostly tried to put a brief numbered list of steps to reproduce that I think sum up the bug here.

+++ b/core/themes/bartik/css/components/primary-menu.css
@@ -200,3 +200,13 @@ body:not(:target) .region-primary-menu .menu-toggle-target-show:target ~ .menu .
+ * Ensures that the open mobile menu hides when the screen dimensions become
+ * bigger than 461px.
...
+@media all and (min-width: 461px) {

Sorry to have to push back on this but "bigger than 461px" doesn't seem accurate (it's actually 461px or wider, not 462 or wider). I think we should say something like "wider than 460px" or "461px or wider".

The patch matches what was asked by @Cottser. I'm marking this RTBC again.

Committed ae233ac and pushed to 8.2.x. Thanks!

Well done all!

Also I forgot to say a big thank you to @Cottser for the thorough review at #31!