search

salcode / bootstrap-genesis

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

Ubermenu Check #5

Closed codenameEli closed 10 years ago

salcode commented 10 years ago

Thanks for the PR, I like this idea. Looking at the changes https://github.com/salcode/bootstrap-genesis/pull/5/files, I think there are some modifications I'd like to see before merging.

  1. GitHub shows "We can’t automatically merge this pull request." Can you make sure you have the latest version from this repo?
  2. Remove the change to javascript.js
  3. Moving class_exists( 'UberMenuStandard' ) inside the bsg_primary_nav_modifications() function and making it a validation check with early return
    i.e. 
function bsg_primary_nav_modifications() {
    if ( class_exists( 'UberMenuStandard' ) {
        return;
    }
    // remove primary & secondary nav from default position
    remove_action( 'genesis_after_header', 'genesis_do_nav' );
    ...

I think this simplifies the code and reduces the number of changed lines we see in the PR (since many of the changed lines are changes in indentation depth).

If you want to make these changes and submit a new PR, that would be awesome. If you've got other things going on, I can take a stab at these changes.

Thanks for contributing.

salcode commented 10 years ago

Superceded by PR #6