salcode / bootstrap-genesis

WordPress Genesis Child Theme setup to use Bootstrap, Sass, and Grunt
MIT License
184 stars 63 forks source link

Add additional layout options #46

Closed bryanwillis closed 9 years ago

bryanwillis commented 9 years ago

This is referring to pull request I wrote for adding additional layouts . I think I did the pull request wrong though. I've done it only once or twice before and was kinda confused about how it worked.

Anyway, for this to work it needs a main genesis() override specifically so genesis_after_content_sidebar_wrap() is moved inside of <div class="content-sidebar-wrap"></div>.

There is probably a better way of doing this, but this seemed like the simplest approach without having to put too much thought into php and overriding too much of genesis and reinforces genesis's semantic markup while still being able to use bootstrap column classes.

There is probably another way to get genesis_after_content_sidebar_wrap() inside the wrap either by unhooking and hooking it earlier or maybe genesis_site_layout() or filtering genesis_attr_- or the genesis-structural-wraps but I was tired last night when I did this and didn't want to mess trying to figure that out.

Anyway, I can't promise that the sass will work because I haven't tried it, but have tried with less and it works fine although could probably be shortened/cleaned up.

This can all be tested here:

content-sidebar sidebar-content content-sidebar-sidebar sidebar-sidebar-content sidebar-content-sidebar fullwidth

Again this was just me trying some things out so this probably isn't the best way to do it, but it seems simple enough and keeps things clean for the most part. It seemed very complicated to create three column layouts using bootstrap sidebar-secondary was outside of the content-sidebar-wrap since custom additional rows would have to be added to allow the nesting.

salcode commented 9 years ago

My gut is I'd like to move all of the layout options into PHP and change the classes being applied based on the chosen layout but I'm not certain about this.

Going this route would include changing how sidebar-content is current handled (in CSS/Sass).

bryanwillis commented 9 years ago

That's what I was thinking initially with PHP, but I was getting irritated how long it was taking to figure that out.

Going this route would include changing how sidebar-content is current handled (in CSS/Sass).

By that do you mean specifically for the sidebar-content layout css:

body.sidebar-content {
    .content-sidebar-wrap {
        main {
            float: right;
            @media (max-width: 767px) {
              float: none;
            }
        }
    }
}

If so is that a big deal? To reorder columns, bootstrap uses left and right css attributes as opposed to floats so a sidebar-content layout that has div which need to be reordered, should "technically" look like this:

<div class="content-sidebar-wrap row">
<main class="col-sm-9 col-sm-push-3" role="main" itemprop="mainContentOfPage" itemscope="itemscope" itemtype="http://schema.org/Blog" id="main-content-container">
</main>
<aside class="col-sm-3 col-sm-pull-9" role="complementary" itemscope="itemscope" itemtype="http://schema.org/WPSideBar">
</div>
bryanwillis commented 9 years ago

Also you are right that the sass version I wrote isn't working. I tried compiling it with codekit on mac and I got the same issue. I only looked at sass for a few minutes the other day so I must have converted it wrong. I will try and figure it out.

salcode commented 9 years ago

I've created a feature branch called feature-layout-choices. I'd like to make these changes on that branch and once we're happy with them merge them all in at once.

salcode commented 9 years ago

@bryanwillis Based on your markup and classes, I modified the classes applied in lib/bootstrap-markup-layout-options.php for sidebar-content, thank you.

I like that this allows us to eliminate CSS and more directly leverage Bootstrap.

We should be able to do the same thing with

but first we need to move .sidebar-secondary inside .content-sidebar-wrap

I think it makes the most sense for .sidebar-secondary to come after .content and .sidebar-primary and then adjust the positioning with the push/pull classes.

salcode commented 9 years ago

I think the scope of this is a little too broad to keep as an issue.

I've added the milestone Layout Options.

From here, I'd like to continue targeting the individual pieces that need to go into place to reach this milestone.