[WIP] [Outdated] Added a first implementation of a BreadcrumbRenderer #45

stof · 2012-04-17T21:05:30Z

This is a WIP about #24 meant for discussion.

This implementation uses the getBreadcrumbsArray method, making it easy to append additional stuff at the end of the breadcrumb. However, it means that you are limited when rendering each item as you only have the label and the uri. The other solution would be to build an array of items or to call getParent each time, but it would make it harder to pass additional paths as it would require building items for them (in this regards, an array of items would be easier than calling getParent during the rendering though as we don't need to add the new items in the existing menu tree).

To use it, you need to call the knp_menu_render method with the item corresponding to the end of the breadcrumbs, i.e. the item for which the breadcrumb is rendered (except the additional path of course). This is a good use case to get an item by path in the tree

stof · 2012-04-17T21:25:39Z

@dbu @uwej711 could you take a look at this ?

stloyd · 2012-04-17T21:40:24Z

src/Knp/Menu/Renderer/BreadcrumbRenderer.php

+
+namespace Knp\Menu\Renderer;
+
+use \Knp\Menu\ItemInterface;


First \ is not required.

indeed. I missed it because I created the file by copying the ListRenderer

cordoval · 2012-04-17T22:10:14Z

does this PR has tests?

stof · 2012-04-17T22:11:49Z

@cordoval as you can see in the diff, there is no tests at all. But I started the description by saying it is a WIP meant for discussion with the CMF guys asking for the feature (and any other guy interested in it)

dbu · 2012-04-18T10:34:52Z

src/Knp/Menu/Renderer/BreadcrumbRenderer.php

+                break;
+        }
+
+        return str_repeat(' ', $spacing).$html."\n";


this is just about readability of the html source code, right?

indeed. This is used to render some nice HTML (which can be changed by setting the compressed option to true, like other renderers)

dbu · 2012-04-18T10:38:43Z

thanks for taking this up! i think its on a good track.

from drupal i remember that you had the option to tell the breadcrumb to not show the current item, or to not link but make it just text. not showing the current menu item would here mean to just render the parent of the current item when calling the twig function, right? should we maybe add an option whether the current item should be linked or not? and anyway add a special class to it so it can be stiled differently?

stof · 2012-04-18T19:24:36Z

Regarding not rendering the current item, it is indeed as easy as passing the parent to the render call.

About adding a special class for the current item, it is a bit hard as we only have the label and the uri currently. So the first point would be discussing if we use getBreadcrumbsArray or if we find a way to turn the additional elements into items too to keep more details.

dbu · 2012-04-20T07:47:20Z

RE special class for current item: you are right. i think we should rather take this and recommend to extend the renderer for special requirements like this.

one question remains: could we use twig templates for the rendering or do you really want to render all in php? (see comment on the code where its looking rather complicated)

but i have no experience with twig extension best practice, maybe the overhead is too big? i don't have a strong opinion but if its doable twig could be nice (also to extend by just replacing a twig template rather than extending the renderer class).

stof · 2012-04-20T08:44:34Z

@dbu My only issue with Twig is the way default options are handled. The BreadcrumbRenderer uses different options, and I would like to avoid duplicating the TwigRenderer for this. So I need to find a better way to handle this (which is why I started with a whole separate Renderer).

What do you think about my point about using item vs using arrays ? Btw, there is an issue with the breadcrumbs array (see the ticket linked above)

dbu · 2012-04-20T08:54:39Z

sounds to me like we should do a bc break for #47 anyway, its a rather surprising and unnecessary constraint to not be allowed the same label twice.

we could define the new structure as array of array (the main array being the breadcrumbs, with insignificant indexes) and the inner array containing fields 'label' and 'url' (or whatever we previously had) and an 'item' that is the item object if there is any, in case you have a custom renderer that does more stuff with it. wdyt?

…tests

dbu · 2012-05-22T10:17:08Z

cool, what is missing to merge this?

stof · 2012-05-22T10:36:12Z

I think it is quite ready. I will look at it this evening. Then, I will need to check how it could be done using Twig without duplicating the renderer

havvg · 2012-05-24T16:03:57Z

I just tried it, and it currently renders nodes that should not be displayed. I applied this patch to get it working.

diff --git a/src/Knp/Menu/Renderer/BreadcrumbRenderer.php b/src/Knp/Menu/Renderer/BreadcrumbRenderer.php
index a7aab60..5d7492b 100644
--- a/src/Knp/Menu/Renderer/BreadcrumbRenderer.php
+++ b/src/Knp/Menu/Renderer/BreadcrumbRenderer.php
@@ -92,6 +92,10 @@ class BreadcrumbRenderer extends Renderer implements RendererInterface
      */
     protected function renderItem($label, $uri, array $options, ItemInterface $item = null)
     {
+        if (null !== $item && !$item->isDisplayed()) {
+            return '';
+        }
+
         $isCurrent = null !== $item && $item->isCurrent();
         $attributes = $isCurrent ? array('class' => $options['current_class']) : array();

stof · 2012-05-24T16:09:32Z

@havvg when an item is not displayed in a menu, none of its children are displayed either. Your change would not do it. And btw, this would mean that you are explicitly asking to display the breadcrumb for a hidden item, so I'm not sure we should hide it in such case.

havvg · 2012-05-24T16:28:26Z

Hm.. then I'm doing it wrong, I guess. I'm retrieving it like this: (Symfony2 + KnpMenuBundle)

This renders the root item + the menu (patch reverted):

{{ knp_menu_render(knp_menu_get('main', ['profile', 'show']), {}, 'breadcrumb') }}

This code does not (default twig renderer), it only renders the children of the root item:

{{ knp_menu_render('main') }}

Did I miss something?

stof · 2012-05-24T16:34:14Z

@havvg I know that the current code shows every node in the breadcrumb. But try to hide the profile node in your example. With your patch, show would still be displayed whereas it does not appear in the full menu either (as its parent is hidden). And your breadcrumb code is explicitly asking to display the breadcrumb for the main / profile / show item. So I would also find it weird to hide it.

havvg · 2012-05-24T17:18:41Z

Seems reasonable; then I'm asking, why the second call is not rendering the root node, but only its children and how may I accomplish this with the breadcrumb, too?

stof · 2012-05-24T17:22:29Z

ah yeah, the breadcrumb array includes the root item. This should probably be changed. This methods needs more changes...

havvg · 2012-05-24T22:21:35Z

src/Knp/Menu/Renderer/BreadcrumbRenderer.php

+    {
+        $this->defaultOptions = array_merge(array(
+            'current_as_link' => true,
+            'current_class' => 'current',


Is it intended to have those options with underscore? The other renderers have their options called currentAsLink, currentClass.

The only place where the library does not follow the Symfony2 standards is for array keys, where underscores should be used (which also leads to a violation of the Twig CS for the same reason). So I'm wondering about changing them to underscores in the v2 of the library, but I'm not sure yet (it would be a BC break)

Imho it should be compatible with the current renderers and break BC with all at once at some version.

Well, the master branch of the library is the dev branch for 2.0. I want to think a bit more about this renaming, and about the issues related to the breadcrumb (see other comments) but such renaming would be done not long after the merge. But I will open a separate issue to discuss it

TomiS · 2012-05-24T22:30:23Z

Hey, this seems to be an interesting effort. Maybe I could get involved somehow too... I finally got it loading after I realized I have to define a service that provides breadcrumb renderer (like this)

services:
    knp_menu.renderer.breadcrumb:
        class: Knp\Menu\Renderer\BreadcrumbRenderer
        tags:
            - { name: knp_menu.renderer, alias: breadcrumb }

Now I'm wondering why it only renders the root node if I leave all extra parameters out. But anyway, it's 1.30 am, so I'm gonna have to look at this more tomorrow.

stof · 2012-05-24T22:31:52Z

As described above, you need to call the render method by passing it the item for which you want to render the breadcrumb, not the root.

TomiS · 2012-05-24T22:39:04Z

@stof Okay, so is there a generic way of retrieving the item for current url/route (from twig point of view)? I guess the path method isn't something that can generally be used because path variables are not 'known' in any random bundle/template that uses the menu.

dbu · 2012-05-30T11:08:56Z

rendering the root node would best be optional. some people want it, others don't.

TomiS · 2012-05-30T12:57:46Z

Yep, I checked that I'd be able to do what I need to do with css selector :last-child. But it's IE9+ only. So not a true option for any real application quite yet. I'm not really sure how this should be solved. I'm just saying, the 'last' class for the last item (how ever it is solved) might be needed.

Mopster · 2012-10-11T09:09:24Z

Any new developments in this PR ? I'd like to use this breadcrumb renderer.

tiagojsag · 2013-01-30T13:01:08Z

+1 for this

maxailloud · 2013-04-11T16:32:08Z

Any news on this PR?
I'm searching a good way to make a breadcrumb.

davidgorges · 2013-05-18T14:55:06Z

+1

jmontoyaa · 2013-05-21T07:28:18Z

+1

mikemeier · 2013-05-22T17:26:58Z

+1

felipep · 2013-06-12T12:46:30Z

+1

dominikzogg · 2013-06-19T12:14:10Z

+1

ahilles107 · 2013-06-20T11:43:29Z

👍

bitsche · 2013-06-28T11:53:19Z

+1

artworkad · 2013-07-26T11:55:38Z

+1

hugohenrique · 2013-07-26T14:08:32Z

this implementation of the breadcrumb was in what state?

artworkad · 2013-08-04T10:36:16Z

KnpLabs/KnpMenuBundle#194

thvd · 2014-01-17T15:47:47Z

+1

jdespatis · 2014-01-23T08:33:22Z

Any news on this feature?

Nek- · 2014-02-28T10:41:07Z

@stof nothing against rebasing on master this branch ?

stof · 2014-02-28T10:44:58Z

@Nek- Actually, I'm not convinced this is the appropriate approach:

normal renderer requires passing the root menu
this breadcrumb renderer requires passing the leaf menu

this could make it really confusing to use. However, I have no real idea about a good implementation.

thus, we don't have yet a way to get the current item easily from Twig (we have a way to get it in PHP thanks to the filter iterator)

Nek- · 2014-02-28T12:01:06Z

@stof can you come on gitter ( https://gitter.im/KnpLabs/KnpMenu ) to speak about the future of KnpMenu ?

stof · 2014-02-28T13:33:18Z

not really during work time. Is it OK for you if we do it this evening or during the weekend ?

Nek- · 2014-02-28T13:47:35Z

It will be perfect at the evening, thanks :-) .

pestaa · 2014-05-06T06:47:18Z

+1

lsmith77 · 2014-08-12T07:10:59Z

i guess #161 aims to address this for 2.1?

Added a first implementation of a BreadcrumbRenderer

ff7fe96

stloyd reviewed Apr 17, 2012
View reviewed changes

dbu reviewed Apr 18, 2012
View reviewed changes

stof mentioned this pull request Apr 19, 2012

getBreadcrumbsArray is broken if a label is used twice in a path #47

Closed

stof added 2 commits May 17, 2012 15:16

Merge branch 'master' into breadcrumb_renderer

4cd6a6a

Updated the BreadcrumbRenderer for the new format, added options and …

bffbbac

…tests

havvg reviewed May 24, 2012
View reviewed changes

maxailloud mentioned this pull request Apr 11, 2013

Access request in a menu listener KnpLabs/KnpMenuBundle#182

Closed

dbu mentioned this pull request May 22, 2012

use breadcrumb once implemented in knp menu symfony-cmf/menu-bundle#3

Open

jmontoyaa mentioned this pull request May 21, 2013

Breadcrumbs KnpLabs/KnpMenuBundle#75

Closed

thvd mentioned this pull request Jan 17, 2014

Breadcrumbs component phiamo/MopaBootstrapBundle#766

Closed

Nek- removed this from the 2.0 beta milestone Mar 17, 2014

Nek- changed the title ~~Added a first implementation of a BreadcrumbRenderer~~ [WIP] [Outdated] Added a first implementation of a BreadcrumbRenderer May 15, 2014

Nek- closed this May 26, 2014

[WIP] [Outdated] Added a first implementation of a BreadcrumbRenderer #45

[WIP] [Outdated] Added a first implementation of a BreadcrumbRenderer #45

Conversation

stof commented Apr 17, 2012

stof commented Apr 17, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cordoval commented Apr 17, 2012

stof commented Apr 17, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbu commented Apr 18, 2012

stof commented Apr 18, 2012

dbu commented Apr 20, 2012

stof commented Apr 20, 2012

dbu commented Apr 20, 2012

dbu commented May 22, 2012

stof commented May 22, 2012

havvg commented May 24, 2012

stof commented May 24, 2012

havvg commented May 24, 2012

stof commented May 24, 2012

havvg commented May 24, 2012

stof commented May 24, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomiS commented May 24, 2012

stof commented May 24, 2012

TomiS commented May 24, 2012

dbu commented May 30, 2012

TomiS commented May 30, 2012

Mopster commented Oct 11, 2012

tiagojsag commented Jan 30, 2013

maxailloud commented Apr 11, 2013

davidgorges commented May 18, 2013

jmontoyaa commented May 21, 2013

mikemeier commented May 22, 2013

felipep commented Jun 12, 2013

dominikzogg commented Jun 19, 2013

ahilles107 commented Jun 20, 2013

bitsche commented Jun 28, 2013

artworkad commented Jul 26, 2013

hugohenrique commented Jul 26, 2013

artworkad commented Aug 4, 2013

thvd commented Jan 17, 2014

jdespatis commented Jan 23, 2014

Nek- commented Feb 28, 2014

stof commented Feb 28, 2014

Nek- commented Feb 28, 2014

stof commented Feb 28, 2014

Nek- commented Feb 28, 2014

pestaa commented May 6, 2014

lsmith77 commented Aug 12, 2014