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

Support for multi-level dropdown #52

Closed
wants to merge 1 commit into from
Closed

Conversation

mharis
Copy link

@mharis mharis commented Nov 28, 2013

No description provided.

@PlanBrewski
Copy link
Collaborator

Thanks for the commit @mharis!

I'm pretty torn on the multi-level dropdown issue. My one issues with merging this is I have been fighting adding support for multi-level dropdowns for quite some time. The reasons for that are:

  1. Its not natively supported by Bootstrap 3. You probably noticed that I purposely made the walker ignore everything over one level deep. This was in an effort to not include unsupported markup.
  2. The script was intended to be a simple drop in and add a few lines of code to your theme, with no additions the the bootstrap framework.

On the other hand it's requested by users every second day. After some thought I think this may be something that we can gracefully integrate into the repo with a few conditions.

  1. The additional files be moved to an add-ons directory. This way its clear that it is not necessary to integrate to use the walker. This also leaves room for other features like my nav-list walker ( https://github.com/twittem/wp-bootstrap-navlist-walker ) So your files would be in add-ons > multi-level-dropdown
  2. The add-on must offer full support for all responsive breakpoints without causing confusion for the user.
  3. The walker must not output additional markup if the add-on is not integrated. My thought on this is adding a function to process add-ons support where you would add something the example below this to your functions.php file to add support for the add-on. We can then use the setting in the walker class to conditionally add functionality, but only when it is intended. In Fact this may be a great way to handle glyphicons.
if ( class_exists( 'WP_Bootstrap_Navwalker' ) ) {
    WP_Bootstrap_Navwalker::add_addon_support( array( 
        'multi-level-dropdown',
        'nav-list' )
    );
}

I would love to know your thoughts on this method. If it is an avenue you agree with I will create a dev branch and we can merge pull request there.

@mharis
Copy link
Author

mharis commented Nov 29, 2013

Hey Edward,

Yes, I am quite aware that BS3 no longer supports multi level dropdowns since it is built for mobile first and multi level dropdowns just doesn't work well on mobiles.

The theme I am working on requires multi-level dropdowns. Multi-level dropdown is much loved and used by our users quite a lot. Its crazy but I have seen many 5 level multi-level dropdown. Personally, I am not a fan of 2nd level or more dropdowns.

I can agree with your thoughts and we should do exactly like you suggest to add support for multi level dropdowns. Next I am working on adding responsive breakpoints to the navigation for multi-level dropdowns. Hit me up on skype, my ID is muhammad-haris and we can talk more about this.

Thanks!

@PlanBrewski
Copy link
Collaborator

I am going to close this pull request. If you want to re-initiate the pull request on the DEV branch I will gladly merge it. Thanks for your work on this.

@PlanBrewski PlanBrewski closed this Dec 3, 2013
@PlanBrewski PlanBrewski mentioned this pull request Dec 30, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants