Comments

playfulwolf’s picture

Issue summary: View changes
m.stenta’s picture

This is an issue when the Bootstrap navbar theme setting is set to "Fixed Top".

markhalliwell’s picture

Title: Incorrect z-index or similar "layering" problem with Navbar module » Navbar integration
Version: 7.x-3.0 » 7.x-3.x-dev
Component: Code » CSS Overrides
Category: Bug report » Feature request
Priority: Normal » Minor

FWIW, integration with smaller modules is generally outside the scope of this project. However, patches are welcome.

Kristina Katalinic’s picture

@markcarver It is indeed a smaller module but I feel that when using Bootstrap theme its convenient to have a responsive admin toolbar provided by navbar module but I know you're busy as it is so I am committing a patch as suggested.
It works with all Bootstrap navbars and on all devices (screenshots attached), and it is consistent with how new responsive admin toolbar looks with Bootstrap fixed navbar menu in Drupal 8 (admin toolbar drops under the navbar).

Kristina Katalinic’s picture

Issue summary: View changes
Kristina Katalinic’s picture

StatusFileSize
new177.75 KB
new181.03 KB
new172.53 KB
new163.92 KB
new155.5 KB

sorry about the last comment was having trouble uploading screenshots of the fix.

Kristina Katalinic’s picture

Kristina Katalinic’s picture

StatusFileSize
new948 bytes

Sorry about the mess. File upload trouble. I have no idea where my original patch disappeared so here it goes again.

Kristina Katalinic’s picture

StatusFileSize
new918 bytes
+++ b/starterkits/less/less/overrides.less
@@ -797,3 +797,22 @@ div.features-export-list {
+  margin-left: 24rem;  ¶

found unnecessary spaces at end of line

+++ b/starterkits/less/less/overrides.less
@@ -797,3 +797,22 @@ div.features-export-list {
+// Navbar module support for fixed-top navbar.

Inconsistent with previous comments format

re-rolling with neater code...

markhalliwell’s picture

Title: Navbar integration » Navbar module integration
Priority: Minor » Normal
Status: Needs review » Needs work
+++ b/starterkits/less/less/overrides.less
@@ -797,3 +797,23 @@ div.features-export-list {
+body.navbar-is-fixed-top #navbar-administration.navbar-oriented,
+body.navbar-is-fixed-top #navbar-administration.navbar-oriented .navbar-bar,
+body.navbar-is-fixed-top #navbar-administration.navbar-oriented .navbar-tray {

Is there a reason the #navbar-administration ID is being used here? It's never a great idea to use IDs as this kind of specificity can cause override issues down the road.

Just looking at the code, it would seem the follow should work, given that Navbar isn't using IDs as well:

body.navbar-is-fixed-top .navbar-oriented,
body.navbar-is-fixed-top .navbar-bar,
body.navbar-is-fixed-top .navbar-tray {

Also, I'm really not that keen on the idea of the Bootstrap navbar being above the admin menu when it's fixed top.

Kristina Katalinic’s picture

@markcarver

body.navbar-is-fixed-top .navbar-oriented,
body.navbar-is-fixed-top .navbar-bar,
body.navbar-is-fixed-top .navbar-tray {

doesn't work because of line 191 of navbar.module.css

body.navbar-fixed #navbar-administration.navbar-oriented, #navbar-administration.navbar-oriented .navbar-bar, #navbar-administration.navbar-oriented .navbar-tray {
    left: 0;
    position: absolute;
    right: 0;
    top: 0;
}

where navbar module is using top:0; same as Bootstrap theme fixed to top navbar header ID. So I was using:

body.navbar-is-fixed-top #navbar-administration.navbar-oriented,
body.navbar-is-fixed-top #navbar-administration.navbar-oriented .navbar-bar,
body.navbar-is-fixed-top #navbar-administration.navbar-oriented .navbar-tray {
  top: 51px;
}

to override navbar modules position clashing with bootstrap .navbar-fixed-top navbar header or else the navbar module is hidden behind bootstrap fixed to top navbar.
The other way would be to adjust bootstrap .navbar-fixed-top position if body has classbody.navbar-administration
which would also then put navbar module admin menu above Bootstrap navbar however position is only part of the problem because override would be required for navbar module admin menu z-index @

#navbar-administration { 
    z-index: auto;

I am happy to continue working on this but let me know which direction you prefer

markhalliwell’s picture

Re: ID selectors: that's unfortunate, but understand. Hopefully someone will clean up that CSS one day so it's not using IDs.

Re: fixed top: While I would prefer that the Bootstrap navbar get pushed down if the Navbar module menu is present, like it currently does when admin_menu is present, I understand that the drawer might make this quite difficult. It just looks rather odd is all. I'm not opposed to putting it in this way if it's going to be too complicated. I wanted to see if it could be attempted at least.

---

One last thing, I'm not too familiar with this the navbar module's markup and CSS. How close is it to what's in 8.x core? Will this patch work for it as well?

Kristina Katalinic’s picture

@markcarver
Re: fixed top; We can push down .navbar-fixed-top position if body has navbar-administration classes and override

#navbar-administration { 
    z-index: auto;

to something like 1031 (fixed to top navbar is using 1030)
And that would bring the navbar administration on top of bootstrap navbar

Re: ID selectors; If we go with the above fix we can drop #navbar-administration selectors as we would be adjusting Bootstrap navbar position with body.navbar-is-fixed-top.navbar-administration .navbar-fixed-top instead.

Re: How close is it to what's in 8.x core? Will this patch work for it as well?; classes are completely different: in 8.x core .navbar-fixed is .toolbar-bar and nav#navbar-administration is nav#toolbar-bar so...
it would have to be a completely new patch but we can match UX that we decide on here if you want to open up a separate issue for 8.x and assign it to me I am happy to work on that one as well

Either way, and this is just my 2c...I do believe user experience with any responsive theme is somewhat diminished without a responsive admin menu, and with 8.x core having a responsive toolbar it kind of redefines the priority for these patches IMHO....

markhalliwell’s picture

We can push down .navbar-fixed-top position if body has navbar-administration classes

Yeah, let's go this route.

edit: new screenshots would be preferred as well so I don't have to manually test

Once we commit this to 7.x-3.x, let's just keep this issue for 8.x and then submit a new patch.

Kristina Katalinic’s picture

@markcarver here is the new patch with admin navbar on top of bootstrap menu

push down .navbar-fixed-top position if body has navbar-administration classes
Yeah, let's go this route.

Unfortunately due to navbar module's own styles use of #navbar-administration was unavoidable in some places

On desktop with Bootstrap fixed to top navbar - navbar admin menu tray was going over the logo instead of pushing to the right like with with bootstrap navbar "normal" or "static top" which is the desired behaviour so I applied

/* Small devices (tablets, 768px and up) */
@media (min-width: @screen-sm-min) {
body.navbar-is-fixed-top.navbar-administration.navbar-vertical.navbar-tray-open .navbar-fixed-top {
  left: 240px;
  left: 24rem;
  }
}

reason I used @media (min-width: @screen-sm-min) is because this did not look good on smarphone devices

I have tested throughly with all Bootstrap navbar variations on both desktop and mobile devices (Xcode Simulator so Apple devices only) and can confirm navbar module admin menu behaviour with Bootstrap theme is consistent with Bartik (except on mobile of course) and Ember (all devices) 8 test screenshots attached

markhalliwell’s picture

Version: 7.x-3.x-dev » 8.x-3.x-dev
Status: Needs review » Patch (to be ported)

Awesome, thanks @Kristina Katalinic!

Changing to 8.x-3.x so a new patch can be created using different selectors.

  • markcarver committed 3758509 on 8.x-3.x
    Issue #2251919 by Kristina Katalinic, markcarver: Navbar module...
markhalliwell’s picture

Status: Patch (to be ported) » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.