Open salcode opened 8 years ago
Yes! You answered my issue I asked about this (fixed it, thank you) and yes I totally think this should be the default, 100%.
I mentioned this idea here awhile back about changing the .wrap
class in structural wraps to .container
.
I think we should reconsider this or doing something similar.
Currently "almost" every aspect of this theme is customizable via actions/filters, with the menu containers being one of the exceptions. True, there's always the option of completely overriding the bsg_nav_menu_markup_filter
, but that would be a TON of code to copy and paste into another function when simply trying to change <div class="container-fluid">
to <div class="container">
.
The obvious answer is just hard code the change on a theme to theme basis, but this defeats a lot of what the theme is trying to accomplish. For example, filters like bsg_navbar_brand
become completely pointless the second we edit the bootstrap-genesis/lib/nav.php
file.
So if we did something like this (this could also be added to bsg-classes-to-add
instead):
add_filter( 'genesis_attr_structural-wrap', 'bsg_attributes_structural_wrap' );
function bsg_attributes_structural_wrap( $attributes ) {
$attributes['class'] = 'container';
return $attributes;
}
We could delete <div class="container-fluid">
all together from nav.php.
Then by simply adding the nav
and subnav
to genesis-structural-wraps
we can let people edit this via a filter...
add_theme_support( 'genesis-structural-wraps', array(
'nav',
'subnav',
//'header',
//'site-inner',
//'footer-widgets',
//'footer'
) );
Then the site developer could override much easier... For example they could change all wrap containers to container-fluids or do things on an individual basis.... Like if they wanted to remove or change one just filter the specific wrap
add_filter( 'genesis_structural_wrap-menu-primary', '__return_empty_string' );
We'd probably be able to get rid of this too then from bsg-classes-to-add
:
'site-header' => 'container',
'site-inner' => 'container',
'site-footer' => 'container',
Anyway, something like this would let people make changes without editing the theme directly (which seems like what we're trying to accomplish). Also, seems like we're taking away a big feature of genesis by not utilizing the wraps.
Update: Adding a helper function would let developers change or remove container
to container-fluid
wraps on the fly. Currently the hard coded container-fluid method were implementing limits the value a lot of the functionality in that file if the developer has to make his changes there instead of functions.php
. We have a filter for the navbar-brand and are even conditionally checking for ubermenu. This is the only part in that entire file that's not overridable with a filter (as far as I'm aware).
So basically if we implement this code below, developers will be able to change and add structural wraps and fluid structural wraps to any part of their site dynamically.
// Replace wrap with container
add_filter( 'genesis_attr_structural-wrap', 'bsg_attributes_structural_wrap' );
function bsg_attributes_structural_wrap( $attributes ) {
$attributes['class'] = 'container';
return $attributes;
}
// Allow for containers to be changed on the fly with a filter
function bsg_wrap_container_fluid( $output, $original_output ) {
if ( $original_output == 'open' ) {
$output = sprintf( '<div %s>', genesis_attr( 'container-fluid' ) );
}
return $output;
}
Then all that's needed to change the menus is this, which could be added to functions.php, templates, etc.
add_filter( 'genesis_structural_wrap-menu-primary', 'bsg_wrap_container_fluid', 16, 2); // .container-fluid primary menu
add_filter( 'genesis_structural_wrap-menu-secondary', 'bsg_wrap_container_fluid', 16, 2); // .container-fluid secondary menu
So it would now look like this:
function bsg_nav_menu_markup_filter( $html, $args ) {
if (
'primary' !== $args->theme_location &&
'secondary' !== $args->theme_location
) {
return $html;
}
$data_target = "nav-collapse" . sanitize_html_class( '-' . $args->theme_location );
$output = <<<EOT
<div class="navbar-header">
<button type="button" class="navbar-toggle collapsed" data-toggle="collapse" data-target="#{$data_target}">
<span class="sr-only">Toggle navigation</span>
<span class="icon-bar"></span>
<span class="icon-bar"></span>
<span class="icon-bar"></span>
</button>
EOT;
if ( 'primary' === $args->theme_location ) {
$output .= apply_filters( 'bsg_navbar_brand', bsg_navbar_brand_markup() );
}
$output .= '</div>'; // .navbar-header
$output .= "<div class=\"collapse navbar-collapse\" id=\"{$data_target}\">";
$output .= $html;
$output .= '</div>'; // .collapse .navbar-collapse
return $output;
}
This would also allow you to add additional items to the navbar dynamically....
Alternatively, the bsg-classes-to-add
way we change:
This came up in #111
Currently the nav goes full width.
Full Width Navigation (current behavior)
Should we change it to be within the
.container
area?Within Container Navigation (new default?)