Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove structural .wrap divs? #63

Closed
salcode opened this issue Mar 13, 2015 · 4 comments
Closed

Remove structural .wrap divs? #63

salcode opened this issue Mar 13, 2015 · 4 comments

Comments

@salcode
Copy link
Owner

salcode commented Mar 13, 2015

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
Copy link
Contributor

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
Copy link
Owner Author

salcode commented Mar 18, 2015

@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
Copy link
Contributor

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
Copy link
Owner Author

salcode commented Mar 20, 2015

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 e91303f 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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants