Dropdown menu label should allow html inside #132

Closed
nysander opened this Issue May 13, 2012 · 17 comments

Comments

Projects
None yet
2 participants
@nysander
Contributor

nysander commented May 13, 2012

to make it possible we need to set such label as html safe. see: KnpLabs/KnpMenu#48

I don't want to break BC but I think that createDropdownMenuItem(ItemInterface $rootItem, $title, $push_right = true) method in abstractmenubuilder will need change to allow extra parameter from menu builder that this label should have html safe parameter set to be able to add such dropdown icon:

<a href="#" data-toggle="dropdown" class="dropdown-toggle">Navigation <b class="caret"></b></a>

(arrow down icon)

atm when you add all label it is safely rendered and html is not parsed

@nysander

This comment has been minimized.

Show comment Hide comment
@nysander

nysander May 13, 2012

Contributor

on this image is my addon to this bundle generating subnavbars, ugly copy of your navbar generator codes http://img19.imageshack.us/img19/782/iconrn.png

as you see dropdown have icon i wanted. my solution for this without breaking BC is here f50fc63 , if you accept it you may pull it or I make a pull request

subnav implementation on https://github.com/nysander/MopaBootstrapBundle/tree/subnav

Contributor

nysander commented May 13, 2012

on this image is my addon to this bundle generating subnavbars, ugly copy of your navbar generator codes http://img19.imageshack.us/img19/782/iconrn.png

as you see dropdown have icon i wanted. my solution for this without breaking BC is here f50fc63 , if you accept it you may pull it or I make a pull request

subnav implementation on https://github.com/nysander/MopaBootstrapBundle/tree/subnav

@nysander

This comment has been minimized.

Show comment Hide comment
@nysander

nysander May 14, 2012

Contributor

I also think about merging navbar code to one and make 3 types of navbars for default. top bar as your one. main bar which looks like top navbar but do not have title element (which is now forced as required parameter) and actual subnavbar. this is 3 different template and atm this would require triplication of code, maybe we work out some way to integrate this in one.

Contributor

nysander commented May 14, 2012

I also think about merging navbar code to one and make 3 types of navbars for default. top bar as your one. main bar which looks like top navbar but do not have title element (which is now forced as required parameter) and actual subnavbar. this is 3 different template and atm this would require triplication of code, maybe we work out some way to integrate this in one.

@phiamo

This comment has been minimized.

Show comment Hide comment
@phiamo

phiamo May 14, 2012

Owner

i also do not like the code to be duplicated, i think i need to play a round a bit with some code, cause i dont want to break anything too.

html in label i would like to omit.
i like the idea of just giving a icon class, or even making a block which could be overridden in an own template.

Owner

phiamo commented May 14, 2012

i also do not like the code to be duplicated, i think i need to play a round a bit with some code, cause i dont want to break anything too.

html in label i would like to omit.
i like the idea of just giving a icon class, or even making a block which could be overridden in an own template.

@nysander

This comment has been minimized.

Show comment Hide comment
@nysander

nysander May 14, 2012

Contributor

html allowed labels are must to have icons set. so my solution is to allow user only define icon class and if he wants it left or right (to be done yet) and escape label content and icon name before entering code to html allowed label.

Contributor

nysander commented May 14, 2012

html allowed labels are must to have icons set. so my solution is to allow user only define icon class and if he wants it left or right (to be done yet) and escape label content and icon name before entering code to html allowed label.

@phiamo

This comment has been minimized.

Show comment Hide comment
@phiamo

phiamo May 14, 2012

Owner

can you split safe label and subnavs into different pull requests / branches please, so i can review them, and make also prs to you

Owner

phiamo commented May 14, 2012

can you split safe label and subnavs into different pull requests / branches please, so i can review them, and make also prs to you

@nysander

This comment has been minimized.

Show comment Hide comment
@nysander

nysander May 14, 2012

Contributor

subnavs is on its own branch, safe label is on initializr branch atm, I'l try to move this commit to its own branch but have not splitted historic tree ever and have no idea how to split now. I can split current initializr branch which contain safelabels commit to be on its own

Contributor

nysander commented May 14, 2012

subnavs is on its own branch, safe label is on initializr branch atm, I'l try to move this commit to its own branch but have not splitted historic tree ever and have no idea how to split now. I can split current initializr branch which contain safelabels commit to be on its own

@nysander

This comment has been minimized.

Show comment Hide comment
@nysander

nysander May 14, 2012

Contributor

you have now branches subnav and safe-label available

Contributor

nysander commented May 14, 2012

you have now branches subnav and safe-label available

@nysander

This comment has been minimized.

Show comment Hide comment
@nysander

nysander May 15, 2012

Contributor

one more thing, icon could be added also in <i> tag but I have no idea if this change will have any matter.

Contributor

nysander commented May 15, 2012

one more thing, icon could be added also in <i> tag but I have no idea if this change will have any matter.

@phiamo

This comment has been minimized.

Show comment Hide comment
@phiamo

phiamo May 15, 2012

Owner

hmm since bootstrap supposes the use of the <i> tag http://twitter.github.com/bootstrap/base-css.html#icons
i would say the i tag should be prefered, but i also think it would be possible to add a $tag = 'i' parameter too ?

Owner

phiamo commented May 15, 2012

hmm since bootstrap supposes the use of the <i> tag http://twitter.github.com/bootstrap/base-css.html#icons
i would say the i tag should be prefered, but i also think it would be possible to add a $tag = 'i' parameter too ?

@nysander

This comment has been minimized.

Show comment Hide comment
@nysander

nysander May 15, 2012

Contributor

I think that this will be good and forward looking solution to allow tag definition with default set 'i'

Contributor

nysander commented May 15, 2012

I think that this will be good and forward looking solution to allow tag definition with default set 'i'

@nysander

This comment has been minimized.

Show comment Hide comment
@nysander

nysander May 18, 2012

Contributor

i've done some rebasing to make it easier to follow code changes and then merging

Contributor

nysander commented May 18, 2012

i've done some rebasing to make it easier to follow code changes and then merging

@phiamo

This comment has been minimized.

Show comment Hide comment
@phiamo

phiamo May 21, 2012

Owner

hmm i had a quick look into your subnav branch (after merging it with master)
i dont like the code duplication from e.g. the original renderer it should be a extended class only changing the needed parts ... this needs refactoring, and i dont have time to do it atm

Owner

phiamo commented May 21, 2012

hmm i had a quick look into your subnav branch (after merging it with master)
i dont like the code duplication from e.g. the original renderer it should be a extended class only changing the needed parts ... this needs refactoring, and i dont have time to do it atm

@nysander

This comment has been minimized.

Show comment Hide comment
@nysander

nysander May 21, 2012

Contributor

Ok if I will have some idea how to do it I'll push to subnav branch and ping for review.

Contributor

nysander commented May 21, 2012

Ok if I will have some idea how to do it I'll push to subnav branch and ping for review.

@phiamo

This comment has been minimized.

Show comment Hide comment
@phiamo

phiamo May 21, 2012

Owner

i merged your safe label branch into master

Owner

phiamo commented May 21, 2012

i merged your safe label branch into master

@phiamo phiamo closed this May 21, 2012

@nysander

This comment has been minimized.

Show comment Hide comment
@nysander

nysander May 21, 2012

Contributor

great thx

Contributor

nysander commented May 21, 2012

great thx

@phiamo

This comment has been minimized.

Show comment Hide comment
@nysander

This comment has been minimized.

Show comment Hide comment
@nysander

nysander May 21, 2012

Contributor

looks a lot better and you see that menu item is dropdown not just button.

Contributor

nysander commented May 21, 2012

looks a lot better and you see that menu item is dropdown not just button.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment