Prefix all navbar classes to prevent theme clashes

I see this as a big blocker. First thing I did when demoing this to a client was setup a clean install with the client's choice of theme (bootstrap). Boom, sad panda.

#1916300: Conflict with Mobile friendly navigation toolbar module has been marked as duplicate.

Ugh. Sad panda indeed.

https://github.com/twitter/bootstrap/blob/master/less/navbar.less

What if we changed the class on the wrapping element from .navbar to .drupal-navbar?

I'm attaching a patch. CashWilliams and andregriffin, can you test this with Bootstrap and see if it resolves the issue?

This is probably a good candidate for forward-porting to D8 as well.

navbar-menu should be safe and not so drupal branded? I like to see this fixed too, but the naming sound not optimal to me.

I recently opened a new issue that may be a dupe of this one. Can someone confirm?

#1955912: Conflict with FontAwesome

I've tested patch #5 against my custom theme that includes the fontawesome css and after an initial run through it's looking good.

Thanks andregriffin for the patch and caschbre for testing. There were a few holes in the RTL files. I've filled them in. The patch in #5 didn't apply cleanly because of changes to Navbar since it was created, so I had to guess at how to apply the changes. Please verify that 3rd party themes are working with the development version of Navbar. If not, a further patch or screenshots will help us get it fixed.

Patch in #8 works well. Super thanks.

See my comment here #1955912: Conflict with FontAwesome for icon classes and fontawesome support.

Thanks

Shouldn't this not wait for the backport #1938044: Prefix all toolbar classes to prevent theme clashes?

Updated navbar to latest dev in my Spark install and it worked fine execpt for the Responsive Preview dropdown. See screenie.

#1938044: Prefix all toolbar classes to prevent theme clashes has been committed. We can follow up now...

I'm going to make an attempt at the backport from Drupal 8 toolbar... I'll post a patch soon.

Attached is a patch against the Navbar 7.x-1.x branch! It was made by walking through the patch from #1938044: Prefix all toolbar classes to prevent theme clashes line-by-line and making the equivalent changes in the navbar module. I tested manually and everything seems to be working correctly. Please let me know what you think!

One random note: it appears as though Panopoly is going to release their 1.0-rc5 soon with Navbar 1.0-alpha10. However, without these changes, that won't work with Open Atrium 2.0 (based on Panopoly) because it uses Bootstrap in it's default theme. If we can get these changes in and a Navbar 1.0-alpha11 made quickly, we could possibly avoid some complications for Open Atrium 2.0.

Here is the issue on the Panopoly issue queue: #1971292: Minor fixes for Navbar module in panopoly_admin.make

Anyway, I'm not trying to rush you. :-) If some patches need to be thrown in some make files for other distributions, so be it. I just thought I'd let you in on some related things happening in other projects.

I just realized that I skipped a patch that was applied to Drupal 8 Toolbar before the one I backported. I've created a Task to backport it too: #2050699: Backport "Reduce dependency on JavaScript" from Drupal 8 toolbar

Patch #14 working for me with latest Panopoly + Radix. Thanks @dsnopek.

@arshadcn: So, what you're saying is "reviewed & tested by the community"? :-)

@jessebearch: Are you able to commit this patch, please? What is holding it back?

hass, I've just yesterday backported all of the D8 Toolbar prefixing we did back to the Navbar. This issue should be addressed through that work in the latest 7.x-1.x dev branch.

Seems not fixed. Found classes

• lining
• slider
• section
• col-2
• section
• col-4
• page-controls

In rtl css files class names like:

• Icon
• Bar
• Vertical

And tons of others.

And many [dir="rtl"] in LTR files

And found an id named toolbar-item what is D8

Thanks for the honesty-check :) Forgot that D7 doesn't have [dir="rtl"] by default. On it.

Where did you find col-4 and col-2?

Oh, in navbar.tpl.php. I don't think that's even being used!

Removed an unused template file. (86fc9147224843dba0429d490532cca9503ebf15)

hass, can you review the 7.x-1.x branch and make sure I got everything?

Thanks for quick fixes. I try my best, but it may need to wait until next weekend.

Found this bug #2119989: Add navbar_menu_tree() to prevent theme clashes

Looks good for now, but please see all the CNR cases I attached patches, too.

Nope, there are still 44 matches for [dir="rtl"] that are not located in rtl CSS files.

All the [dir="rtl"] references are deleted and the RTL files are updated. I think everything should be good now.

Which patches specifically, hass? I'll have a look through.

There's no new patch so I guess you simply rolled the changes into the latest dev? I also discovered the hard way that if you upgrade navbar to dev, you also have to upgrade breakpoints (see #1938162: Drupal Update Is Unreliably HORRIBLE).

In any case I still don't see any improvement in navbar with the Bootstrap theme.

Check out if the .menu class is the root cause of all your issues. It's the only remaining critical issue, but that is a different case

@koppie, can you provide a screenshot of the issues? I'll load the theme as well, but having a screenshot will help me understand the specific issues you are running into.

I loaded up Bootstrap 3. The only major issue I noticed is the width of the menu tray is too narrow. I made a commit to change the width unit from rem to em to solve this.

0a52368206c1609cb882a0adab37a8ee8851af9d

Otherwise, everything looks to be styled within an acceptable range, no?

Please ignore the arrow in the image below. It's not meant to point at anything. The process of uploading and referencing an image is to cumbersome for me to run through it again :/

I'm seeing the same problem as @busa in #11; screenshot attached.

@koppie and @busa, the Bootstrap theme overrides theme_item_list and removes the <div class="item-list"></div> wrapper around the ul.

theme_item_list is a system theme function. It's kind of like an API for front end code. If you change this API, you're going to get unexpected behavior. So in this case, I consider this a bug with Bootstrap, not Responsive Preview which is depending on Drupal system-level theme functions.

Check out /theme/system/item-list.func.php in bootstrap and log an issue about this in the theme. If you get this div back into the theme function, Responsive Preview will work, otherwise you'll have to skip this module or override Bootstrap's override of theme_item_list to make it match the system output again.

I found two issues with Bootstrap that are overrideable with CSS.

I've committed them as: 2ee91d225409edfac7e785c1f812974640637275 and 5590dc8c9279e040244799360485d23a4d6cdf87. The code is:

/**
 * Undo overzealous theme resets.
 */
+.drupal-navbar *:before,
+.drupal-navbar *:after {
+  -webkit-box-sizing: content-box;
+  -moz-box-sizing: content-box;
+  box-sizing: content-box;
+}

This theme really needs to be less global about its resets :/

And

- .drupal-navbar .navbar-toggle-orientation button {
+ .drupal-navbar .navbar-toggle-orientation .navbar-toggle {
      cursor: pointer;
      display: inline-block;
+   margin: 0;
+   padding: 0;
}

I'm going to set this issue to fixed. If we find specific problems from here on out, we can log them separately.