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

Added submenuOptions property to Dropdown #7427

Closed
wants to merge 4 commits into from

Conversation

spikyjt
Copy link
Contributor

@spikyjt spikyjt commented Feb 24, 2015

A submenu options array adds flexibility to the dropdown, allowing custom attributes to be added to submenus. This patch doesn't break BC, as it sets submenuOptions to options, if it has not been explicitly set. However it does get the rid of the dropdown-menu class that is currently set on submenus, causing them to be hidden by default.

A submenu options array adds flexibility to the dropdown, allowing custom attributes to be added to submenus. This patch doesn't break BC, as it sets submenuOptions to options, if it has not been explicitly set. However it does get the rid of the dropdown-menu class that is currently set on submenus, causing them to be hidden by default.
@cebe cebe added this to the 2.0.x milestone Feb 24, 2015
@cebe
Copy link
Member

cebe commented Feb 24, 2015

You have a failing test here: https://travis-ci.org/yiisoft/yii2/jobs/52027817#L917 can you please fix it?

@spikyjt
Copy link
Contributor Author

spikyjt commented Feb 24, 2015

Looks like that fixed it. Sorry, I'm new to unit tests. They're still on my todo list to get familiar with!

@samdark
Copy link
Member

samdark commented Feb 25, 2015

Test doesn't look right now.

@spikyjt
Copy link
Contributor Author

spikyjt commented Feb 25, 2015

@samdark do you think that the submenu should have the class dropdown-menu? Having this class is incorrect as far as Bootstrap is concerned. You can't have a dropdown inside a dropdown. You can have a submenu, Bootstrap just doesn't provide styling for them by default. Removing the dropdown-menu class from the submenu makes it behave as expected, i.e. shown by default with the parent dropdown, instead of being hidden with no way to show it.

@samdark
Copy link
Member

samdark commented Feb 25, 2015

Ah, then it's probably fine.

@spikyjt
Copy link
Contributor Author

spikyjt commented Feb 25, 2015

Thanks @samdark . The test is definitely now correct as far as my code change is concerned. As long as you are all happy with the logic of my code change, then all is well.

@spikyjt
Copy link
Contributor Author

spikyjt commented Feb 25, 2015

Should I add to the test to include settings for the new submenuOptions property?

@samdark
Copy link
Member

samdark commented Feb 25, 2015

Yes, that would be great.

@spikyjt
Copy link
Contributor Author

spikyjt commented Feb 26, 2015

Ok, my new test passes, but I'm wondering if I should make submenuOptions a property of each item, rather than the whole widget. That way you could set it differently per level, if required. Perhaps I should have created an issue for this rather than just a PR?!

@klimov-paul
Copy link
Member

@spikyjt, can you please resubmit this PR up to yii2-bootstrap repo?

@klimov-paul
Copy link
Member

Migrated to yiisoft/yii2-bootstrap#41

@cebe cebe removed this from the 2.0.x milestone May 15, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants