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

Breadcrumb support/example #11

Closed
dereuromark opened this issue Apr 11, 2020 · 18 comments · Fixed by #12
Closed

Breadcrumb support/example #11

dereuromark opened this issue Apr 11, 2020 · 18 comments · Fixed by #12

Comments

@dereuromark
Copy link
Contributor

Add an example for breadcrumb from current site tree defined, and current "active" item found.
Should be possible right?

I am wondering if the data for this tree could also be coming from a DB and GUI backend
Where you define it via DB tables and Tree behavior and then just let it generate the "menu" tree of the library, and let it then match/work.
Would that make sense for a larger app with many controllers? As it could auto-find possible Tree items and then propose already a CRUD default. Then you just need to arrange and customize it.

@ndm2
Copy link
Member

ndm2 commented Apr 11, 2020

Sure, that should be possible, the helper doesn't care about where the data comes from, I have for example a CMS running where the menu structure can be built in the backend, which the helper uses to generate the menu.

It's just a matter of configuration, and overcoming possible limitations with regards to rendering, like for example adding screen reader only data for active items (that's of course not limited to breadcrumbs).

I'm not sure though what exactly you're asking for with regards to the support/example? Support for gathering site tree information? An example that builds a navigation from dynamic data?

@dereuromark
Copy link
Contributor Author

Last part, an example in docs here, would suffice for most cases.

@ndm2
Copy link
Member

ndm2 commented Apr 13, 2020

I see. I'm not really sure what such an example should look like, I can't think of anything other than iterating over a set of records, but is that really something that needs to be explained? Maybe for threaded data an example of recursive iterating would be helpful.

Do you have any specific problems in mind that should be covered?

@dereuromark
Copy link
Contributor Author

Well, I saw some open PRs on the main repo that haven't been processed for some reason

but I also saw that there is already existing functionality inside, e.g. https://github.com/KnpLabs/KnpMenu/blob/13e3e32946e24427fdc8074eda7d704f6e4b28e7/src/Knp/Menu/Util/MenuManipulator.php#L225

@ndm2
Copy link
Member

ndm2 commented Apr 13, 2020

Oh, so you're talking about creating breadcrumbs from an existing menu?

Ie when you have a menu with nested items, and want to create breadcrumbs for a specific path, being it the path of a manually chosen node, or the one that holds the current active item (assuming the menu is configured in a such a way that there is only a single active item)?

@dereuromark
Copy link
Contributor Author

That is now the question. If the docs could recommend a strategy, e.g. when you have the active item, and thus the path back to the root element.
I think one then just passes it on to the Html helper method and it should work this way.

@ndm2
Copy link
Member

ndm2 commented Apr 13, 2020

I'll have a look at it, surely the menu helper could provide functionality to assist with that, like extracting paths, finding the active item, etc.

@ndm2
Copy link
Member

ndm2 commented Apr 14, 2020

Ok, this revealed quite some flaws, given that the "current" state of an item isn't stored in the menu items themselves (unless marked manually) but in the matcher, being able to retrieve the current item would require quite some changes so that the helper retains the matcher used for the menu. This won't play nicely with custom renderers, and it might require the menu to be rendered first in any case, as people are allowed to pass custom matchers to MenuHelper::render().

There seems to have been quite some discussion about retrieving the current item over at the KnpMenu repository, and the consensus seems to be to iterate over all menu items and issue matching checks (again), using the matcher that is used for rendering the menu.

Furthermore for a breadcrumb navigation, I certainly see the possibility that people would like to include the menus root item, however it's currently not possible to pass menu item specific configuration options to that item.

That's quite some problems that need to be solved carefully in order to not create lots of side effects and possibly more problems further down the road. Long story short, it will unfortunately take some time 😞

@ndm2
Copy link
Member

ndm2 commented Apr 17, 2020

Looking at this further, I think support for determining the current item will need to be split into minor an major changes.

As a first step in the next minor, determining the current item by matching again (ie no matcher reuse unless the matcher is created and passed manually) could be introduced (to be fair, it's relatively inexpensive, matching through 1000 items with the UriVoter takes about 0.0006 seconds on my not overly optimized dev machine).

In a second step in the next major, which would take some more time, reusing matchers could be introduced, which will require quite some breaking changes.

@ndm2
Copy link
Member

ndm2 commented Apr 19, 2020

If you're interested, here's a first implementation for extracting paths and retrieving the current item:

https://github.com/icings/menu/tree/4.x/feature/paths-and-current-item

Quick example, say you have a menu like this:

$mainMenu = $this->Menu->create('main');
$mainMenu->addChild('Library', ['uri' => ['controller' => 'Library', 'action' => 'index']]);
$mainMenu['Library']->addChild('Web', ['uri' => ['controller' => 'Web', 'action' => 'index']]);
$mainMenu['Library']['Web']->addChild('Data', ['uri' => ['controller' => 'Data', 'action' => 'index']]);
$mainMenu->addChild('Projects', ['uri' => ['controller' => 'Projects', 'action' => 'index']]);
$mainMenu->addChild('Settings', ['uri' => ['controller' => 'Settings', 'action' => 'index']]);
// ...

You could extract/create a breadcrumbs navigation from it based on the current item like this:

$crumbs = [];
$currentItem = $this->Menu->getCurrentItem('main');
if ($currentItem) {
    $crumbs = $this->Menu->extractPath($currentItem);
}

$crumbsMenu = $this->Menu->create('breadcrumbs', [
    'templates' => [
        'menu' => '<ol class="breadcrumb"{{attrs}}>{{items}}</ol>',
    ],
    'currentAsLink' => false,
]);

$crumbsMenu->addChild('Home', ['uri' => ['controller' => 'Pages', 'action' => 'display', 'home']]);
foreach ($crumbs as $crumb) {
    $crumbsMenu->addChild($crumb);
}

As can be seen, an additional Home item is prepended for the crumbs navigation. Often breadcrumb navigations do have a "Home" entry, however as mentioned earlier, the root item of a menu currently isn't configurable via the menu creation options, and I'm not sure whether it actually should be, as it kind of represents the menu wrapper element, not a menu entry element.

Assuming the Data item is the current item, the above example would render the following HTML for the breadcrumbs menu:

<ol class="breadcrumb">
    <li>
        <a href="/">Home</a>
    </li>
    <li>
        <a href="/library">Library</a>
    </li>
    <li>
        <a href="/library/web">Web</a>
    </li>
    <li class="active">
        <span>Data</span>
    </li>
</ol>

@dereuromark
Copy link
Contributor Author

dereuromark commented Apr 19, 2020

I am currently thinking of using a DB backend and build in your plugin as frontend part.
https://github.com/dereuromark/cakephp-navigation

Right now I can auto extract already controller actions and build a basic tree menu from it (convention based with index/first action as root of each controller).

Now I just need to extract the tree, and programmatically create the Menu as per your API and I should have menu + breadcrumbs then as DB driven solution.

@ndm2
Copy link
Member

ndm2 commented Apr 19, 2020

I see, that should work fine. I can push a quick 3.x backport of it if you'd like to try it with your plugin?

@dereuromark
Copy link
Contributor Author

If you are interested to support this also in 3.x, that would be most welcome.

@ndm2
Copy link
Member

ndm2 commented Apr 19, 2020

Sure, for the time being the 3.x branch will receive the same feature updates as the 4.x branch if possible... and while I'm saying that I notice that I still haven't backported the class inheritance/consuming feature 😬

@dereuromark
Copy link
Contributor Author

I see that it uses v2 vs v3 of the menu builder library. There arent that big of differences?

@ndm2
Copy link
Member

ndm2 commented Apr 19, 2020

Not really, AFAICT there aren't any new features or notable changes to the API, it's mainly the requirement of PHP 7.2+, introduction of strict types, and dropping deprecated code.

@ndm2
Copy link
Member

ndm2 commented Apr 19, 2020

Here we go: https://github.com/icings/menu/tree/3.x-feature/paths-and-current-item

Now I'll have to think long and hard about the root item stuff before I'll continue this.

@ndm2
Copy link
Member

ndm2 commented Jun 4, 2020

So after thinking about this for some time, I cam to the conclusion that keeping the current behavior of root menu items is the best way forward for now.

I'll go over this again, add some docs, and publish releases for 4.x and 3.x soon.

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

Successfully merging a pull request may close this issue.

2 participants