It would be nice to have this admin toolbar to not take the generic "navbar" class, as many other front-end themes may use this class, causing many styling conflicts. This is especially a problem with Twitter Bootstrap based themes.

Comments

cashwilliams’s picture

Priority:Normal» Major

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.

hass’s picture

jessebeach’s picture

Status:Active» Needs review
StatusFileSize
new19.42 KB

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.

hass’s picture

Version:7.x-1.0-alpha1» 7.x-1.x-dev

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.

andregriffin’s picture

StatusFileSize
new19.18 KB

Almost. There were also some conflicts with the .icon class, but adding some "auto" values for width and height, and background: none; seemed to fix everything. Also pushing out a commit soon for Bootstrap to prevent our theme_menu_link function from doing dropdown-menu stuff on the management menu.

caschbre’s picture

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

#1955912: Conflict with FontAwesome

caschbre’s picture

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

jessebeach’s picture

StatusFileSize
new22.72 KB

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.

arshadcn’s picture

Patch in #8 works well. Super thanks.

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

Thanks

hass’s picture

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

busla’s picture

StatusFileSize
new40.76 KB

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

hass’s picture

Title:Add class specificity to allow other themes to use generic "navbar" class» Prefix all navbar classes to prevent theme clashes
Status:Needs review» Needs work

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

dsnopek’s picture

Assigned:Unassigned» dsnopek

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

dsnopek’s picture

Status:Needs work» Needs review
StatusFileSize
new37.54 KB

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!

dsnopek’s picture

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.

dsnopek’s picture

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

arshadcn’s picture

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

dsnopek’s picture

Status:Needs review» Reviewed & tested by the community

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

hass’s picture

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

jessebeach’s picture

Status:Reviewed & tested by the community» Closed (duplicate)

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.

hass’s picture

Status:Closed (duplicate)» Needs work

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

jessebeach’s picture

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

jessebeach’s picture

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

jessebeach’s picture

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

jessebeach’s picture

StatusFileSize
new2.28 KB

Removed an unused template file. (86fc9147224843dba0429d490532cca9503ebf15)

jessebeach’s picture

Status:Needs work» Needs review

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

hass’s picture

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

hass’s picture

hass’s picture

Status:Needs review» Fixed

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

hass’s picture

Issue summary:View changes
Status:Fixed» Needs work

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

jessebeach’s picture

Status:Needs work» Needs review

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.

koppie’s picture

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.

hass’s picture

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

jessebeach’s picture

@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.

jessebeach’s picture

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 :/

jessebeach’s picture

StatusFileSize
new95.44 KB
jessebeach’s picture

Issue tags:+beta-blocker, +rc-blocker
jessebeach’s picture

Issue tags:-beta-blocker
koppie’s picture

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

koppie’s picture

StatusFileSize
new42.14 KB
jessebeach’s picture

@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.

jessebeach’s picture

Assigned:dsnopek» Unassigned
jessebeach’s picture

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;
}
jessebeach’s picture

Status:Needs review» Fixed

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

Status:Fixed» Closed (fixed)

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