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

Add support for setting the CSS class for the menu and its items #15

Closed
wants to merge 1 commit into from

Conversation

crisu83
Copy link

@crisu83 crisu83 commented Mar 29, 2015

Currently it isn't possible to configure the menu CSS class nor the item CSS class, which mean that this widget cannot be used for creating other menus, such as list groups, which is a shame.

I added two new properties to the widget; menuCssClass and itemCssClass, which allows us to use this widget for other bootstrap menus than those containing the nav CSS class.

I would also suggest considering to rename the Nav class to Menu in the future.

@cebe
Copy link
Member

cebe commented Mar 29, 2015

There is a Menu class in the framework already which basically does the same as Nav. Can you check whether it fits for list groups?

@crisu83
Copy link
Author

crisu83 commented Mar 29, 2015

I already did and it kind of does, with the exception that you cannot configure the menu item CSS class. Instead you need to manually set it for each item which is undesirable (IMHO). I'm not sure I understand why the Nav class doesn't extend the Menu class, even though they both contain very similar logic?

I think this same functionality should be available in the Menu class as well, but it seemed more natural to add it to the Bootstrap package as there you have a real use-case for it. That said I think this PR follows very much the same idea as the firstItemCssClass and lastItemCssClass found in the Menu class.

@cebe
Copy link
Member

cebe commented Mar 29, 2015

I'm not sure I understand why the Nav class doesn't extend the Menu class, even though they both contain very similar logic?

Yeah, I asked the same question myself too. I think this should be refactored in 2.1

@crisu83
Copy link
Author

crisu83 commented Mar 29, 2015

Agreed, I think that's the best solution. However, I still think we should support setting the menu and item class classes as I proposed in this PR.

@cebe cebe added this to the 2.0.4 milestone Mar 29, 2015
@cebe
Copy link
Member

cebe commented Mar 29, 2015

sure, set for milestone 2.0.4

@crisu83
Copy link
Author

crisu83 commented Mar 29, 2015

Also as the author of the Yii-Bootstrap and Yiistrap extensions I've also spend quite a lot time thinking about whether we should support setting CSS classes through variables or if it's overkill for a framework to provide that support.

While it might seem convenient it makes the code a bit more complex and adds some potentially unnecessary logic to the widgets. I guess you just need to decide whether you want it or not.

@crisu83
Copy link
Author

crisu83 commented Mar 29, 2015

You should also fix the build, it seems that it runs composer update which is never a good idea, instead it should simply run composer install with the necessary flags and read the committed lock file.

@klimov-paul klimov-paul added the type:enhancement Enhancement label Apr 21, 2015
@klimov-paul
Copy link
Member

@crisu83, cna you please get pull from upstream and add a changelog line?

@samdark samdark modified the milestones: 2.0.5, 2.0.4 May 11, 2015
@@ -116,7 +126,7 @@ public function init()
if ($this->dropDownCaret === null) {
$this->dropDownCaret = Html::tag('b', '', ['class' => 'caret']);
}
Html::addCssClass($this->options, 'nav');
Html::addCssClass($this->options, $this->menuCssClass);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such construction does not seem right: we have a $options, which allows specification of the 'class' and $menuCssClass for the specification of the class.
It seems either Html::addCssClass() usage should be abandonded or menuCssClass not introduced.

@klimov-paul
Copy link
Member

Resolved by #52

@cebe cebe removed this from the 2.0.5 milestone May 28, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants