salcode / bootstrap-genesis

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

Remove structural .wrap divs? #63

Closed salcode closed 9 years ago

salcode commented 9 years ago

We are not using the Genesis structural wrap divs, div.wrap, within Bootstrap Genesis, however this is a starter theme and I'm wondering if others often use them when building a theme. In particular, building something like a full-width header or footer comes to mind?

I realize they can be added back easily enough but if they're often used, I'm tempted to keep them in.

@codenameEli, @dustyndoyle, @tbcorr Do you have objections to removing these by default? No reply will be assumed to be agreement on removing them.

See patch #61

bryanwillis commented 9 years ago

Been thinking about this one a lot and how we should go about it...

I think the most important thing here is to decide how we plan on going forward with the layout markup, but more specifically I think we need to make a clear decision as to how Bootstrap fits into Genesis. This is of course just an opinion, but I think this would help get more people get involved and be on the same page moving forward. Obviously we're still in the "figuring things out" phase, but I think the sooner we do that the better!

For instance, the layout option milestones is a good example. You mentioned here that you'd like to go forward with the layout options using php. In that case I think moving forward we should focus of Bootstrap-Genesis should be making the markup look like Bootstrap's while using Genesis. If we wan't to do it mostly with css, then the focus should be integrating Bootstrap, but keeping Genesis as "Genesis" as possible. This probably sounds kind of dumb while reading, but I think it's important if we draw out some guidelines so people now how to get involved with the project.

This all goes along with your particular question here because we could look at the wraps two ways here. We should try to integrate wraps into bootstrap-genesis just how they are with regular genesis. The question is which route to take...

For instance, If we want the markup to be more like Genesis we should change the .wrap css to match bootstraps .container css using sass/less

Less version (not sure if this is everything):

.wrap {
  .container-fixed();

  @media (min-width: @screen-sm-min) {
    width: @container-sm;
  }
  @media (min-width: @screen-md-min) {
    width: @container-md;
  }
  @media (min-width: @screen-lg-min) {
    width: @container-lg;
  }
}

.wrap {
  > .navbar-header,
  > .navbar-collapse {
    margin-right: -@navbar-padding-horizontal;
    margin-left:  -@navbar-padding-horizontal;

    @media (min-width: @grid-float-breakpoint) {
      margin-right: 0;
      margin-left:  0;
    }
  }
}

If we want to go the "bootstrap markup" direction we could turn wraps into containers by actually changing the wrap genesis_attr to container...

<?php
remove_filter( 'genesis_attr_structural-wrap', 'genesis_attributes_structural_wrap' );
add_filter( 'genesis_attr_structural-wrap', 'bootstrap_genesis_attributes_structural_wrap' );
function bootstrap_genesis_attributes_structural_wrap( $attributes ) {
  $attributes['class'] = 'container';
  return $attributes;
}

Both of these we could even add container-fluid. Anyway, if we can how we what we want to integrate bootstrap markup moving forward, I think these questions will be easier for us to figure out. What's your thoughts?

salcode commented 9 years ago

@bryanwillis, thanks for the through provoking notes.

Based on offline conversations and my own thoughts, I agree with you that we should remove the structural wraps. Quickly looking at some links out there

they all seem to be adding, not removing.

Are structural wraps adding somewhere else in the theme which we can remove?

A quick search didn't turn up "genesis-structural-wraps" anywhere. I want to be certain we're not adding and removing the feature (rather than simply not adding it in the first place).

I do like the idea of changing the .wrap divs to a Bootstrap class. Something like documenting how to do this (perhaps to achieve a full width header or footer) would be valuable to add in README.md or the currently unused https://github.com/salcode/bootstrap-genesis/wiki

I've also opened #69 to address the broader questions you've brought up.

bryanwillis commented 9 years ago

Are structural wraps adding somewhere else in the theme which we can remove?

By default genesis checks to see if your child theme has support for structural wraps and if not then it automatically adds it. You're right there doesn't seem to be any documentation on removing the wraps on the internet. Pretty surprising.

Genesis tries to check if you have added support or not on line 63 in genesis/lib/init.php

if ( ! current_theme_supports( 'genesis-structural-wraps' ) )
add_theme_support( 'genesis-structural-wraps', array( 'header', 'menu-primary', 'menu-secondary', 'footer-widgets', 'footer' ) );

Because of this if you attempt to use remove_theme_support( 'genesis-structural-wraps' ); in genesis_setup it doesn't appear to work.

So here are the options I've come up with:

function bsg_remove_genesis_structural_wraps() {
    remove_theme_support( 'genesis-structural-wraps' );
}
add_action( 'after_setup_theme', 'bsg_remove_genesis_structural_wraps', 11 );

However even easier (and since you like limiting amount of code we could simply do the following and trick genesis into thinking were adding theme support.

add_theme_support( 'genesis-structural-wraps', '' );

In terms of performance, I'm not sure if either method is better than the other.

// If we want to add it only to specific areas we could do this
function bsg_remove_some_genesis_structural_wraps() {
    remove_theme_support( 'genesis-structural-wraps' );
    add_theme_support( 'genesis-structural-wraps', array( 'post' ) );
}
add_action( 'after_setup_theme', 'bsg_remove_some_genesis_structural_wraps', 11 );

Personally I like this the best:

add_theme_support( 'genesis-structural-wraps', '' );

My reasoning is that it can be added anywhere. So you could add it to https://github.com/salcode/bootstrap-genesis/blob/develop/lib/genesis-setup.php and the user will still be able to add wraps again at the end of functions php and it will take precedence over the one in genesis-setup.php.

salcode commented 9 years ago

Now that I understand Genesis is adding structural wraps for certain elements by default, I like your original patch #61

Adding

remove_theme_support( 'genesis-structural-wraps' );

directly in lib/genesis-setup.php works for me when I test it. This works because in e91303f010195 we changed to loading all the lib files on hook genesis_setup hook at priority 15. This way all of these files hold off until loading at this point in the process.

I like your original remove_theme_support() over add_theme_support( 'genesis-structural-wraps', '' ); because by using remove_theme_support we are emphasizing we are removing the structural wraps, which I think will be helpful for others trying to understand the code (and for myself in the future when I forget what this code does, ha ha).