Panopoly Theme provides two things:

  1. A smattering of small theme customizations that really only make sense in the context of Panopoly, and
  2. A set of really awesome responsive Panels layouts.

I think it really makes sense for those two things to be seperated, since I think #2 would be very useful even outside of Panopoly.

Luckily for us, Radix Layouts includes all the same layouts as Panopoly, but is theme and distribution independent!

Here is what I'm proposing:

  • Radix Layouts becomes a dependency of Panopoly Theme
  • We turn all of Panopoly Theme's layouts into stubs that just call into Radix Layouts (to make it a seamless upgrade, no matter how the layout is specified: in code or overridden in the database)
  • We put something in the release notes and contact all the child distributions based on Panopoly that we know, and tell them that the Panopoly layouts are deprecated in favor of the Radix Layouts

Later we can think about removing the old Panopoly layouts, but I'm not too worried about that. There will be no maintanence of them needed (because they are just stubs), CTools plugins are only loaded when used and the code weight will be minimal.

Remaining TODO

Comments

arshadcn’s picture

I agree with this approach. And I can help with this.

dsnopek’s picture

Issue summary: View changes
caschbre’s picture

@dsnopek... how would you make a panopoly_theme layout just a stub pointing to the radix layouts? If you can provide an example, I could probably get the rest up and running.

mpotter’s picture

Would love to see this relatively soon.

For Open Atrium we will rely on Radix Layouts branches to switch between Bootstrap 2 and Bootstrap 3 support. So we need to get these layouts out of Panopoly Themes.

@arshadcn: any progress on this?

mpotter’s picture

Here is the code I used in Open Atrium's template.php to override Panopoly layouts with Radix layouts:

function oa_radix_ctools_plugin_post_alter(&$plugin, &$info) {
  if (($info['type'] == 'layouts') && ($plugin['module'] == 'panopoly_theme')) {
    if (strpos($plugin['theme'], 'radix_') === FALSE) {
      $plugin['theme'] = 'radix_' . $plugin['theme'];
      unset($plugin['css']);
    }
  }
}

Don't have time right now to make this into a real patch, but adding this code to Panopoly Theme and then adding

projects[radix_layouts][version] = 3.0-alpha2
projects[radix_layouts][subdir] = contrib

to the panopoly_theme make file.

Although I'm not exactly sure what version of Radix layouts is needed since it depends on the version of Bootstrap being used.

caschbre’s picture

@dsnopek and I discussed on IRC what the plan of attack would be. Here's what we were thinking.

1. Modify the panopoly_theme layouts .inc files to point to the radix_layouts layouts folders/files. This would basically mimic what radix_layouts_ctools_plugin_post_alter() attempts to do but by modifying our .inc files we can hopefully avoid confusion if another dev is looking at panopoly_theme layouts.

2. Remove unnecessary files from panopoly_theme layouts. These include .css, .png, and .tpl.php files leaving us with just the .inc file.

3. Enable all radix layouts in the Layouts Library and disable all Panopoly layouts. This doesn't technically disable the layout, it just removes it from the layout selection admin UI.

Issues
The radix layouts assume a bootstrap theme. Everyone else is going to be SOL. The reason for this are the layout .css files included with panopoly_theme. These handle very basic responsiveness for the panopoly_theme layouts. By removing them the pages start to break.

So even if a custom theme is not directly using our custom css classes, the fact that our layouts .css files do causes they're remove to break non-bootstrap themes.

caschbre’s picture

Assigned: Unassigned » caschbre
dsnopek’s picture

The radix layouts assume a bootstrap theme. Everyone else is going to be SOL. The reason for this are the layout .css files included with panopoly_theme. These handle very basic responsiveness for the panopoly_theme layouts. By removing them the pages start to break.

According to Arshad, Radix Layouts bundles the Bootstap grid css, so it should work without actually using a Bootstap based theme. Looking at the code, this at least appears to be the case:

http://cgit.drupalcode.org/radix_layouts/tree/radix_layouts.css

Is it possible the radix_layouts.css isn't being loaded with the way we're doing the stubs for panopoly_theme?

Anyway, because this grid CSS is bundled, nothing should break. If it does, it's a bug somewhere.

caschbre’s picture

FileSize
1.37 MB

Ok, I did some more digging and here's what I've found....

The content pages look ok. Slight variation in column widths but I'll take a look at those later. The real issue is with the landing page (home page). We're displaying a list of content and each item in that list is using a panel. That panel contains the .container class which is set to be the maximum width. Here's a side-by-side screenshot. Each item in the list is using the boxton layout to display the teaser display mode. All of our panels have the container class on them and the radix_layouts.css file sets the .container class to 1170px width.

side-by-side

dsnopek’s picture

Whoa, that's no good! Layouts definitely need to be nestable. I don't recall this being an issue with Radix Layouts 2.x, which I've tested in the past. It might be worth looking at what else is different between 2.x and 3.x (besides just the Boostrap version). But this does seem like a bug in radix_layouts.

@cashbre: Thanks for doing that testing and making the side-by-side screenshot, that's awesome!

caschbre’s picture

The 2.x version does not have the fixed width on the .container class so that is new in 3.x. I'm not familiar enough with bootstrap to know if this is how it should be. I did open a ticket on radix_layouts.

#2334871: Radix Layouts breaks Panopoly for nested panels (e.g. teaser lists)

caschbre’s picture

Not patches for panopoly yet, but there are two new patches for radix_layouts and radix (theme) that will eventually be necessary to make this transition.

#2353825: Avoid nested container class by moving it to page.tpl.php instead of layouts.
#2334871: Radix Layouts breaks Panopoly for nested panels (e.g. teaser lists)

My local testing looks good for the nested container issue. If folks can apply those two patches and see how it looks will help.

caschbre’s picture

Here's a screenie of the home page with both of those radix/radix_layouts patches applied.

Note that the images are not floating left anymore. That's an issue with radix that we'll have to address eventually.

radix container fixes

caschbre’s picture

arshadcn’s picture

Status: Active » Needs review
FileSize
351 bytes

Added radix_layouts to make file.

dsnopek’s picture

Status: Needs review » Needs work

Thanks, @arshadcn!

We're also going to need to remove the templates and CSS for the old layouts in panopoly_theme. @caschbre: You had some code for this working a couple months ago, right? I remember discussing it on IRC. It'd be great if you could update @arshadcn's patch to include that as well.

caschbre’s picture

I'm pretty swamped with a couple of other projects heading into the holidays but I'll see what I can do. I'm assuming we'll also need to update radix_layouts to v3.1 so we can get the nested container fix in there?

dsnopek’s picture

Actually, if you can just post what you had from before (even if its incomplete), we'll be sprinting all day tomorrow and can try to finish it up!

arshadcn’s picture

arshadcn’s picture

Status: Needs work » Needs review
FileSize
351 bytes

New patch with radix_layouts 3.1

dsnopek’s picture

Assigned: caschbre » dsnopek

Sweet, thanks! I'm working on the rest of the panopoly_theme changes to turn the old Panopoly layouts into stubs and hide them from the user. I should have a patch to post for testing soon.

dsnopek’s picture

Ok! Here is my first pass at clearing out panopoly_theme and making the old layouts be stubs to the Radix layouts by default (even without the hook_ctools_plugin_post_alter() in radix_layouts).

Still TODO:

  • Fix the styling when looking at the Radix layouts in the change layout dialog to match the way the Panopoly ones currently look
  • Hiding the "Panopoly" category of layouts from the user so the Panopoly layouts are just there to provide backcompat
  • Add a hook_update_N() to get radix_layouts enabled on old sites

I'm going to try and get both those things done today so we can test this at the sprint today!

klu’s picture

Thanks @dsnopek. I can try to test this patch today as part of the sprint.

@arshadcn and @dsnopek:

Posted a comment+patch in 2334871: Radix Layouts breaks Panopoly for nested panels (e.g. teaser lists)

The mdo fix on Github should work too, but as mentioned by mdo at https://github.com/twbs/bootstrap/issues/11436#issuecomment-29580770

negative margin for the nesting would probably screw up some lives

If using container-fluid, I'm not seeing any styles for container-fluid in radix_layouts.css, so it might be less unpredictable for folks who will start using Radix Layouts (as part of Panopoly with this proposed switch) but are not familiar with the Radix theme?

Per Bootstrap 3:

Use .container for a responsive fixed width container.

Use .container-fluid for a full width container, spanning the entire width of your viewport.

Just a suggestion. Of course, I will defer to both of you for your wisdom and expertise.

caschbre’s picture

@dsnopek: that patch looks nearly exactly what I was working towards. Awesome! The other thing I had was updating the variable that shows/hides layouts for selection. I set the panopoly_theme layouts to hidden and the radix layouts to show. I then did something (don't remember what) that kept the panopoly_theme layouts from being enabled again.

dsnopek’s picture

Ok! Here are patches to panopoly_theme, panopoly_magic and panopoly_core to do the full set of changes. I haven't completely tested them, but I want to post them here so I can try on Travis-CI before doing too much manual testing.

dsnopek’s picture

I then did something (don't remember what) that kept the panopoly_theme layouts from being enabled again.

This sounds really interesting! I think we should do it. If you have any ideas on how you did that, that'd be great. Might be a patch to panopoly_admin because that's what provides the enable/disable layouts page.

klu’s picture

After some additional testing, here's what I found (following @caschbre's example, attaching a comparison screenshot).

Screenshot Example #1: Original -- Responsive Bartik; No Radix Layouts
#1 is what the home page currently looks like before adding Radix Layouts

Latest dev of Radix Layouts works properly with the Radix theme; however, with Responsive Bartik as the default theme, the content will be shifted to the right. (This could be fixed if someone adds the "container" class to each region in page.tpl.php, but that assumes a change to the theme itself.)

Screenshot Example #2: Responsive Bartik - Latest Radix Layouts with Container + mdo fix

With container-fluid, the home page works fine with Responsive Bartik (I also tried my Zen5-based theme).

Screenshot Example #3: Responsive Bartik; Radix Layouts + Container Fluid

With container-fluid, I see that Radix is breaking at #content, but in my tests that could be addressed by adding another "container" class, e.g. something like:

    <div id="content" class="<?php print (!$is_panel) ? 'container' : ''; ?>">
      <div class="container">  <!-- Add container class -->
        <?php print render($page['content']); ?>
      </div>
    </div>

...which would then require something like:

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

My quick test of the above is Screenshot Example #4: Radix Theme + Container Fluid with adjustment

I'm not very familiar with the Radix theme (and I'm new to Radix Layouts), so Arshad may have more to add, but in my tests switching to Radix Layouts would cause issues with non-Radix themes including Responsive Bartik.

arshadcn’s picture

The issue with Responsive Bartik is that it assumes a #main that is wider than .container. And every theme that does that might require small css fixes. But other than that radix_layouts should work with most custom themes.

dsnopek’s picture

The issue with Responsive Bartik is that it assumes a #main that is wider than .container. And every theme that does that might require small css fixes. But other than that radix_layouts should work with most custom themes.

Hrm. Well, we need Responsive Bartik to look right. Is there something we can put into panopoly_theme to fix this?

dsnopek’s picture

Status: Needs review » Needs work

So, I ran the automated tests with the patches on this issue and got a bunch of failures:

https://travis-ci.org/dsnopek/panopoly/jobs/40349218

Basically, we depend on the regions having layout specific CSS classes like "bryant-sidebar". We might be able to work around this using the generic CSS names that are in there (like "sidebar" and "content") but I haven't tried it yet.

dsnopek’s picture

Ok, here is a patch that should fix the tests! Turned out to be way simpler than I thought it would. :-)

dsnopek’s picture

Status: Needs work » Needs review
FileSize
154.96 KB
1.01 KB

Here is a couple new iterations on the patches here. I decided to actually pull the CSS that modifies the way the Radix layouts look out of panopoly_magic and moved it into panopoly_theme, since those layouts come from panopoly_theme. Also, I had to add some CSS to undo some styling that Bartik and Responsive Bartik put on the ".sidebar" class which radix_layouts uses.

All that we need now is:

  1. A new version of radix_layouts which fixes #2334871: Radix Layouts breaks Panopoly for nested panels (e.g. teaser lists)
  2. More testing of these patches by people wo are not me :-)
arshadcn’s picture

1. New Radix Layouts 7.x-3.2 is up.
2. Looks good for me as well.

dsnopek’s picture

Status: Needs review » Needs work

Thanks, @arshadcn!

At BADCamp earlier today @cboyden got these patches setup on her dev site and we walked through it and it was working for her. In talking through it, I realized that we also need to update panopoly_pages to directly using the Radix Layouts, rather than depending on the stubs forwarding from the Panopoly layouts to the Radix ones. I'd also like to quickly do manual testing of each of the layouts just to double check that there are no regressions. I'll try to get this done at the airport right now, but no promises. :-)

dsnopek’s picture

FileSize
8.85 KB

While testing I found weird styling issue with the "Add" button on the "Add content" dialog:

A change is probably necessary to some CSS in panopoly_magic...

dsnopek’s picture

Here are some updated patches! For panopoly_pages and panopoly_demo, it's switching to using the Radix layouts directly rather than the stubs in panopoly_theme. For panopoly_theme, I fixed an issue with the size of the layout icons on the change layout dialog. I still haven't figured out the issue in #35.

klu’s picture

Thanks @dnsopek! I've confirmed that Radix Layouts 3.2 is working. A quick manual test of the patches in #36 on a default Panopoly site appears to work OK (with the already-identified pending items noted). We'll do some additional testing on our distro and see if we find anything else.

dsnopek’s picture

Issue summary: View changes

Looked through comments and my notes to add a "Remaining TODO" section to the issue summary. We're super close! I'll try to update the patches today, and then poke some people for additional help testing. :-)

dsnopek’s picture

Here are some updated patches! I've checked off the things in the issue summary that this fixes.

I also dug into the weird styling issue with the "Add" button on the "Add Content" dialog from comment #35. This is caused by Bootstrap making everything into box-sizing: border-box. Also found a number of other little styling issues connected to this change in the "Add Content" dialog: (a) fieldset/legend doesn't line up the way it should, (b) the scrollbar in the dialog isn't flush with the right side of the dialog.

We can definitely update our CSS to take this into account, but I'm a little worried about the affect this could have on unsuspecting themes. If someone has a theme that's coded with box-sizing: content-box in mind, and then we just switch every element to border-box it could create problems... I'm not sure what to do about it at this point, I'll need to think more on it.

EDIT: My original (a) above was a red herring - replaced it with a real issue...

dsnopek’s picture

I've added an issue to radix_layouts to try and figure out what to do about the box-sizing: border-box problem: #2373565: Restrict box-sizing changes to just the layout classes?

I'd love some input there!

annagaz’s picture

We have done additional tests using the Radix layouts on our distribution! Thus far, we have not found any regressions/issues with the layouts (outside of the border-box issue noted in this ticket).

klu’s picture

@dsnopek: I've tested and commented on the border-box issue.

dsnopek’s picture

Issue summary: View changes
FileSize
31.36 KB

I started going through each layout one-by-one, to try and find any regressions or issues. Unfortunately, I did find a couple little things:

  1. #2374625: radix_sanderson and radix_sanderson_flipped have the same icon!
  2. #2374631: radix_webb and radix_webb_flipped are reversed!
  3. On radix_taylor and radix_taylor_flipped in responsive_bartik, the "Half Column" region has some unexpected bottom margin. I think this is responsive_bartik applying special styling to the ".content" class. This should probably be fixed in panopoly_theme, along with the other bartik fixes. (Note: I've since found more layouts this affects, like radix_harris - but those two example are enough to dig into the problem)

So, I've updated the list of TODO to cover these remaining things! If anyone else has the time to run through all the layouts and check them, it would be much appreciated, since I only had time to do it relatively quickly.

dsnopek’s picture

Hmm. I thought more about point (3) above in #43 and I think it would be best to try and fix that problem in radix_layouts, so I opened a new issue there: #2374637: Prefix classes with commonly used names, like content, sidebar, header, footer?

I appreciate input on that one, because I'm not totally sure about it!

klu’s picture

arshadcn’s picture

All related issues fixed. If the latest dev works I can tag a new release.

Thanks everyone.

dsnopek’s picture

@arshadcn: Thanks so much! Sorry, I got busy on Friday and then was away from the internet all weekend. I'm going to look at this now!

dsnopek’s picture

Status: Needs work » Needs review
FileSize
124.8 KB

Woohoo! Everything looks great. :-) I'm uploading one last patch which removes some responsive_bartik hacks that are no longer necessary (since Arshad committed #2374637: Prefix classes with commonly used names, like content, sidebar, header, footer?) and updates to the latest radix_layouts.

I'm going to run this all through the tests on Travis-CI one last time (hence why the patch uses radix_layouts from Git) and assuming that's good, I'll finally commit this once Arshad does the radix_layouts 3.3 release!

Thanks again to everyone who helped working on this and testing, and especially to Arshad for all his hard work on radix and radix_layouts!

dsnopek’s picture

FileSize
1.11 KB

Well, it's good I decided to try with Travis-CI, because I need to update the panopoly_test patch and I found some minor CSS class name inconsistencies in radix_layouts. The new panopoly_test patch is attached and here is an issue for the radix_layouts: #2376635: Minor class name inconsistencies

Here's the next Travis-CI build:

https://travis-ci.org/dsnopek/panopoly/builds/41252637

dsnopek’s picture

Alright! Travis came back and everything is now looking good. So, all that we need now is #2376635: Minor class name inconsistencies to committed and radix_layouts 3.3 to be released. :-)

arshadcn’s picture

  • dsnopek committed 8a60d2d on 7.x-1.x
    Update Panopoly Core, Theme, Magic, Admin, Users, Pages, Search, Test...
dsnopek’s picture

Status: Needs review » Fixed

Committed! Huzzah! Thanks again everyone. :-)

Just in case, here is the Travis-CI build for the final committed version (which I did modify a little on commit - mostly switching layouts to the radix version, which I missed on my last pass through):

https://travis-ci.org/panopoly/panopoly/builds/41297212

I'll see you all over in #1987386: Include Radix theme in Panopoly ;-)

Status: Fixed » Closed (fixed)

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