Re: #2191069: Have Panopoly Theme depend on Radix Layouts (rather than providing it's own)

Panopoly is attempting to switch to using radix_layouts as the default instead of maintaining our own layouts. Our initial testing is showing some bad results for certain use-cases. (see link for side-by-side screenie).

A normal content page looks ok for the most part, but when looking the demo home page things start to go wrong. What's going on is on the demo home page we display a list of content using the Teaser display mode. The teaser display mode is also using panels to handle the layout. So in essence we have nested panel layouts. Each layout contains the .container class which has a fixed width of 1170px.

Should the .container class have a fixed width like this?

CommentFileSizeAuthor
#34 radix_layouts-switched-containers-to-container-fluid-2334871-34.patch20.67 KBshadcn
#31 radix_layouts-switched-containers-to-container-fluid-2334871-31.patch20.67 KBshadcn
#25 radix_layouts-use_container_fluid-2334871-25.patch20.67 KBklu
#10 container.png102.3 KBshadcn
#9 radix_layouts-remove_container_classes-2334871-9.patch61.39 KBcaschbre
#8 panopoly_radix_layouts_radix_theme.jpg347.26 KBcaschbre
#7 radix_layout_no_container.png444.2 KBcaschbre

Comments

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented

It's these lines in radix_layouts.css which are causing problems:

@media (min-width: 768px) {
  .container {
    width: 750px;
  }
}

@media (min-width: 992px) {
  .container {
    width: 970px;
  }
}

@media (min-width: 1200px) {
  .container {
    width: 1170px;
  }
}

Are those from Bootstrap? Or were they added for Radix Layouts? Anyway, setting a fixed width outside of the theme seems wrong... Maybe max-width was intended?

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Issue summary: View changes

Attempting to add image to issue description.

shadcn’s picture

shadcn commented
Assigned: Unassigned » shadcn

I just noticed this issue.

@dsnopek, the css are from Bootstrap. It looks like there has been a fix for that. Looking into it.

boabjohn’s picture

boabjohn commented

Hi guys, just checking for any updates here...it's a bit of a killer.
Thanks,
JB

caschbre’s picture

caschbre commented

So according to the bootstrap docs it looks like the .containers can not be nested. This broke in bootstrap 3.0.1 when the .container changed from a max-width property to a width property.

What I'm thinking is that the panel layouts should not contain the .container class. The .container class already exists in the page.tpl.php file which should be sufficient.

I'll try this out later and see if that works and create a patch.

boabjohn’s picture

boabjohn commented

Wow...that's a big change on the part of Bootstrap. Thanks heaps for digging in to track it down. Eagerly awaiting the chance to test a patch!
Kind regards,
JB

caschbre’s picture

caschbre commented
StatusFileSize
new radix_layout_no_container.png444.2 KB

So I did some in-browser testing. Basically just used chrome inspection tools to remove the container class from the panel and everything started to fall into place. Granted this was using the responsive bartik theme and not the radix theme so I can do an actual comparison against the original screenies.

It'll probably be a few days until I can get a patch put together.

Here's a screenie that can be compared to the original screenies.

radix layout without the container class

caschbre’s picture

caschbre commented
StatusFileSize
new panopoly_radix_layouts_radix_theme.jpg347.26 KB

More investigative notes...

The original testing was done with radix_layouts + responsive_bartik. Here's a screenie with radix_layouts + radix. As you can see the nested container issue isn't apparent, but that's more because radix works around the issue with the css below. I think this will be problematic as it only focuses on containers within certain view modes and doesn't really address the nested container issue.

I'm still thinking the better solution is to move the .container class out of the layouts and into page.tpl.php on the content div.

.view-mode-featured .container,
.view-mode-teaser .container,
.view-mode-full .container {
  width: auto;
  padding: 0;
}

radix layouts w/ radix theme

Also note that the content list doesn't have the images floated left anymore. The following css overrides the panopoly styling. 

/* line 9, ../../extensions/compass_radix/stylesheets/compass_radix/_node.scss */
.field img.panopoly-image-full,
.field img.panopoly-image-half,
.field img.panopoly-image-quarter {
  max-width: 100%;
  width: auto;
  height: auto;
  float: none;
  margin: 0;
}
caschbre’s picture

caschbre commented
Status: Active » Needs review
Related issues: +#2353825: Avoid nested container class by moving it to page.tpl.php instead of layouts.
StatusFileSize
new radix_layouts-remove_container_classes-2334871-9.patch61.39 KB

Attached is a patch that removes the .container classes from all of the layouts. This goes along with the patch in #2353825: Avoid nested container class by moving it to page.tpl.php instead of layouts. that adds the container class in page.tpl.php on the div that wraps the layouts. This should help avoid the nested containers.

This doesn't solve the images not floating anymore but I think that should be logged as a separate issue to the radix theme itself.

shadcn’s picture

shadcn commented
StatusFileSize
new container.png102.3 KB

Hi

So the reason we have the .container class inside the panels layout is to allow full control over the site layout and not be restricted by what is added to page.tpl.php.

If you have a .container inside the page.tpl.php then you're restricted by how flexible your panels layouts can be. As anything you add to your panels layout will be contained inside the .container from page.tpl.php.

container

This is particularly useful when all your page layouts are controlled using panels. For pages not using panels, we solve this issue as follows (see both links below):

  1. https://github.com/arshad/radix/blob/7.x-3.x/template.php#L116
  2. https://github.com/arshad/radix/blob/7.x-3.x/templates/page/page.tpl.php...

Regarding the fix mentioned in #8, this is the case when you're using panels inside panels (Panelizer). Bootstrap 3 does not allow nested containers anymore, so in Radix we had to fix this using CSS.

Anyone looked into .container-fluid?

caschbre’s picture

caschbre commented

@arshadcn... thanks for the explanation on why the container class is inside the panel. That makes sense but unfortunately doesn't resolve the issue of the nested containers and getting radix_layouts into panopoly.

It sounds like we need to a) get the nested container fix from Radix and put it in radix_layouts, and b) make that fix more generic because it doesn't catch all of the potential nested scenarios.

Can we just do the following in radix_layouts? (granted the -15px would be a variable in sass)

.container .container {
  margin-left: -15px;
  margin-right: -15px;
}

The alternative would be to somehow have a panel aware if it is nested or not and optionally include the .container class.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented

@arshadcn:

Bootstrap 3 does not allow nested containers anymore, so in Radix we had to fix this using CSS.

Unfortunately, because the fix is in Radix, this means that Radix Layouts only works with Radix. In order to include it in Panopoly, Radix Layouts has to be able to work in any theme (well, any theme where it's CSS doesn't conflict with Bootstrap).

I don't know if @cashbre's proposal is viable (because I just don't know Radix/Radix Layouts/Bootstrap that well), but something along those lines seems good.

shadcn’s picture

shadcn commented

We built Radix Layouts to be independent of Radix. In Radix, we just added fixes where we need them. But seems that those fixes might be better in Radix Layouts itself.

I'm going to look into this again tomorrow. I'll let you know what I find out.

Thanks for your help on this @cashbre and @dsnopek.

lsolesen’s picture

lsolesen commented

I've tried rebuilding a theme using Radix - and I only succeed when @cashbre pointed me to this issue, so the .container could be removed from radix layouts. They worked well for reasonable padding, but forced the width of nested layouts :/

morseCode’s picture

morseCode commented

#9 worked like a charm for me. Just had to add a .container to the page.tpl.php file in the my subtheme so the outer div was constrained by the boostrap container settings.

shadcn’s picture

shadcn commented

From the issue on Bootstrap repo, the fixed width for the containers were addded to work with IE8.

Does Panopoly work with IE8? If not. we could override Bootstrap's defaults and use max-width instead.

shadcn’s picture

shadcn commented

Here's a fix inspired from mdo's fix on Github:

.container {
  .container {
    width: auto;
    margin-left: -15px;
    margin-right: -15px;
  }
}
shadcn’s picture

shadcn commented

This is now in the latest Radix dev. Just tested with the latest Panopoly. Looking good.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented

I'd be for the fix in #17, not so much because I understand the issue, but because @caschbre suggested the same thing in #11. If two people independently came to the same conclusion, maybe it's a good idea :-)

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented

This is now in the latest Radix dev. Just tested with the latest Panopoly. Looking good.

Is this going to come into radix_layouts? We need this fix for any theme that uses radix_layouts, not just radix.

shadcn’s picture

shadcn commented

Right. Let me get this in radix_layouts first. I'll send an updated patch.

  • arshadcn committed 5e9f695 on 7.x-3.x 
    Issue #2334871: Fix for nested containers
shadcn’s picture

shadcn commented
Status: Needs review » Fixed

With the latest fix in, nested containers now works with panopoly + radix_layouts.

I'll go ahead and mark this as fixed. Feel free to re-open.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented

Works for me, thanks!

klu’s picture

klu commented
StatusFileSize
new radix_layouts-use_container_fluid-2334871-25.patch20.67 KB

Hi @arshadcn and @dsnopek.

We have a Zen5-based theme for our Panopoly-based distro, and I've been testing Radix Layouts with our theme.

The mdo fix on Github should work, but following Arshad's suggestion in #10:

Anyone looked into .container-fluid?

I tested container-fluid, and that appears to work well too.

Attaching a patch for consideration: radix_layouts-use_container_fluid-2334871-25.patch (note: my first d.org patch, so apologies if any issues)

Is there a reason not to use container-fluid that I'm not aware of? (Our distro theme currently isn't using Bootstrap, but I've used Bootstrap 2 and 3 on non-Drupal projects and have used container-fluid for full width containers).

shadcn’s picture

shadcn commented

The main reason for not using container-fluid is that we're trying to get the provided layouts to work with both panels layouts (when you are using layouts for your whole page) and panelizer layouts (when you're using the layout inside a panel region i.e nested containers)

Fluid containers breaks the first case and works for the second but will still needs margin fixes since there's still a 15px padding on both sides.

I'd rather go with .container (as was intended by the Bootstrap maintainers when they released 3.0) and with the fix we addded to radix_layouts.

klu’s picture

klu commented

Thanks Arshad. In my tests, container fluid works in both cases for me if I'm using Radix Layouts but not using the Radix theme. I'm finding an issue with your latest dev when using Radix Layouts without the Radix theme. Please see my post at https://www.drupal.org/node/2191069#comment-9321695.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Status: Fixed » Active

So, I was under the impression that only .container had the breakpoints where it would drop columns below if the browser window was below a certain size, and that .container-fluid didn't. However, I just did some testing with Kathleen's patch to convert all the layouts to container fluid, and the breakpoints all worked fine!

Given that Responsive Bartik (and probably other themes) seem to have issues with using .container, and .container-fluid seems to work fine, couldn't Radix just put a fixed width wrapper in it's page template and work fine with .container-fluid?

It just seems better to fix one theme (Radix) rather than force all other non-Radix themes to add tweaks...

shadcn’s picture

shadcn commented

@klu and @dsnopek, I'm going to run some tests with container-fluid tonight. I'll post results here.

caschbre’s picture

caschbre commented

@arshadcn... would it make sense to have a setting that radix_layouts could look for to change from .container to .container-fluid? .container can be the default, but that would allow themes/modules to alter what is used.

shadcn’s picture

shadcn commented
StatusFileSize
new radix_layouts-switched-containers-to-container-fluid-2334871-31.patch20.67 KB

Ok. So it seems container-fluids will work better in our case. I've added a container wrapper to page.tpl.php as mentioned by @klu in previous comments.

Here's a patch that switches all containers to container-fluid for all Radix Layouts.

I've already made the changes in the latest Radix dev to support this change.

@klu, can you run your tests again? Thanks.

shadcn’s picture

shadcn commented
Status: Active » Needs review
dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Status: Needs review » Needs work

Sweet, thanks, @arshadcn!

+++ b/plugins/layouts/radix_bartlett/radix-bartlett.tpl.php
@@ -11,7 +11,7 @@
 <div class="panel-display bartlett clearfix <?php if (!empty($classes)) { print $classes; } ?><?php if (!empty($class)) { print $class; } ?>" <?php if (!empty($css_id)) { print "id=\"$css_id\""; } ?>>
-  <div class="container">
+  <div class="container-fluid-fluid">
     <div class="row">
 
       <!-- Sidebar -->
diff --git a/plugins/layouts/radix_bartlett_flipped/radix-bartlett-flipped.tpl.php b/plugins/layouts/radix_bartlett_flipped/radix-bartlett-flipped.tpl.php

I spotted a minor type-o in the patch: container-fluid-fluid (two "fluids")

shadcn’s picture

shadcn commented
Status: Needs work » Needs review
StatusFileSize
new radix_layouts-switched-containers-to-container-fluid-2334871-34.patch20.67 KB

New patch

shadcn’s picture

shadcn commented

Let's get this committed today.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Status: Needs review » Reviewed & tested by the community

Sorry it took me so long to get back to you! It's easy to get distracted at BADCamp. :-) I've tested this patch and it's working for me both under responsive_bartik and the latest Radix -dev!

shadcn’s picture

shadcn commented
Status: Reviewed & tested by the community » Fixed

Just tagged new version with the patch. Going to mark this as fixed.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented

Huzzah! Thanks for all your work on this, Arshad! :-)

klu’s picture

klu commented

Thanks @arshadcn! Sorry I was offline for a couple days. Confirming that everything is working for me! I did a quick test on:

1. Default Panopoly: Responsive Bartik, Radix, Zen-7.x-5.5
2. Our Panopoly-based distro: Zen-7.x-5.5, Custom theme (based on Zen5) and multiple sub-themes

Many thanks again, Arshad!

  • arshadcn committed 5e9f695 on 8.x-3.x 
    Issue #2334871: Fix for nested containers

Status: Fixed » Closed (fixed)

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