diglactic / laravel-breadcrumbs

Laravel Breadcrumbs - A simple Laravel-style way to create breadcrumbs.
https://packagist.org/packages/diglactic/laravel-breadcrumbs
MIT License
868 stars 63 forks source link

Avoid repetition by providing current URL #44

Closed jaulz closed 2 years ago

jaulz commented 2 years ago

First of all, thanks a lot for this great library!

However, while working with it I noticed that I am repeating myself over and over again:

  Breadcrumbs::for('explore.index', function (Generator $trail) {
    $trail->push('Explore', route('explore.index', []));
  });

I have to specify the route basically twice: once in the parameter of for and then once again as a parameter of push. Would you accept a PR where the URL is either defaulted when it's empty or maybe even better have a method on Generator to return it? Something like this:

  Breadcrumbs::for('explore.index', function (Generator $trail) {
    $trail->push('Explore', $trail->currentRoute());
  });

Let me know if you are open for a PR and if yes what the preferred name for the currentRoute method would be.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

shengslogar commented 2 years ago

Sorry I missed this! Agree on the redundancy, but something like currentRoute wouldn't be ideal since the Generator trail isn't aware of route parameters. Maybe you thought of that already?

In the meantime, have you considered something like this?

<?php

use Diglactic\Breadcrumbs\Breadcrumbs;
use Diglactic\Breadcrumbs\Generator as BreadcrumbTrail;

Breadcrumbs::macro('forSelf', function (string $routeName, array $routeParams, string $label) {
    Breadcrumbs::for($routeName, function (BreadcrumbTrail $trail) use ($routeName, $routeParams, $label) {
        $trail->push($label, route($routeName, $routeParams));
    });
});

Breadcrumbs::forSelf('explore.index', [], 'Explore');
stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

jaulz commented 2 years ago

Thanks for your response! That's already a great idea and helps for some cases where the params and name are static but as soon as they are dynamic I need some form of callback again. In the end I think it would be even better to have something like this:

$trail->pushCurrent('Title');

But as you said the Generator is not aware of the current route so it would definitely some other tweaks as well.

shengslogar commented 2 years ago

For dynamic breadcrumb labels: I haven't tested this but you should be able to pass a callable method into your macro, and then using app()->call, inject request dependencies like Route at runtime to then hook into a route-bound model.

<?php

use Diglactic\Breadcrumbs\Breadcrumbs;
use Diglactic\Breadcrumbs\Generator as BreadcrumbTrail;
use Illuminate\Routing\Route;

Breadcrumbs::macro('forSelf', function (string $routeName, array $routeParams, callable $generateLabel) {
    Breadcrumbs::for($routeName, function (BreadcrumbTrail $trail) use ($routeName, $routeParams, $generateLabel) {
        $trail->push(
            app()->call($generateLabel),
            route($routeName, $routeParams)
        );
    });
});

Breadcrumbs::forSelf(
    'explore.index',
    [],
    fn(Route $route) => $route->parameter('someRouteBoundModel')->attribute
);

You should be able to make a "pushCurrent" method like this:

<?php

use Diglactic\Breadcrumbs\Breadcrumbs;
use Illuminate\Support\Facades\Route as RouteFacade;
use Diglactic\Breadcrumbs\Generator as BreadcrumbTrail;

Breadcrumbs::macro('pushCurrent', function (string $label) {
    Breadcrumbs::for(RouteFacade::currentRouteName(), function(BreadcrumbTrail $trail) use($label){
        $trail->push($label, Request::url());
    });
});

Breadcrumbs::pushCurrent('Test');

The problem is breadcrumbs are unique by route name, so this could get messy quickly. Another more dangerous-but-dynamic option is configuring breadcrumbs on-the-fly in a controller or Blade file.

There feels like too many permutations here to make one nice clean addition to the library, but if you end up with a really generic macro I'd be open to discussing adding it directly.

jaulz commented 2 years ago

Thanks a lot for your inputs! I think I will go for the classic approach in the meantime and come back to this in case it gets too messy... 😊 Anyway, would it make sense to make Generator also macroable?

shengslogar commented 2 years ago

Hmm, I'd consider that. What methods asides from a Generator::pushCurrent() might you use there?

jaulz commented 2 years ago

I could think about adding some project specific helpers that fills $data with some predefined values, e.g. pushHighlighted (calling push but automatically pass a highlighted flag in $data so I can highlight that breadcrumb because it's a "main" page).

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.