breadcrumbs icon indicating copy to clipboard operation
breadcrumbs copied to clipboard

Version 2.0

Open dwightwatson opened this issue 4 years ago • 8 comments

This PR is working on a new major release of breadcrumbs, supporting the later versions of Laravel with a brand new API. It'll take advantage of the new improvements in Laravel and drop unnecessary complexities.

Breadcrumbs::for('home', function () {
    $this->then('Home', route('home'));
});

Breadcrumbs::for('users.index', function () {
    $this->extends('home')->then('Users', route('users.index'));
});

Breadcrumbs::for('users.show', function (User $user) {
    $this->extends('users.index')->then($user->full_name, route('users.show', $user));
});

Some key differences:

  • We don't need to pass $trail in as the first parameter anymore - it's bound to the context as $this.
  • We replace parent and add with extends and then which I think are more readable - not sure yet. May keep the old names in for those with preferences.

dwightwatson avatar Apr 22 '20 09:04 dwightwatson

Hi @dwightwatson , are you not considering adding breadcrumbs to your routes, for example, davejamesmiller/laravel-breadcrumbs#209?

tabuna avatar Apr 22 '20 19:04 tabuna

Please tell me, these changes will affect

Breadcrumbs::for('home', function (Generator $trail) {
    $trail->then('Home', route('home'));
});

Right now I have ide tips for the passed object. Are they saved using $this?

I am also worried about the possibility of using short functions:

Breadcrumbs::for('home', fn (Generator $trail) =>
    $trail->then('home')->add('About')
);

Will there be problems using $this? Tips in class/etc

tabuna avatar Apr 22 '20 19:04 tabuna

Registering the breadcrumb directly on a route is an interesting idea. I don't love the idea of a breadcrumbs.php file but don't really have a better idea for it yet. Tying it straight to the route makes sense because they're build off of the route name, but I do worry that it will add a tonne of noice to the routes file.

Not sure about IDE tips - will need to look into that. However, it's my understanding that $this will work fine with short functions.

Breadcrumbs::for('home', fn() => $this->then('home', route('home')));

Might need to rethink the method then as it's a little awkward when it's the first breadcrumb.

There's a lot of ideas here basically - but now that DJM's breadcrumbs package has been archived I'd like to bring mine up to speed so I can swap it out in my newer apps.

dwightwatson avatar Apr 22 '20 23:04 dwightwatson

The IDE will definitely give the wrong hints if it is used in any class:

class AppServiceProvider extends ServiceProvider
{
    public function boot()
    {
        Breadcrumbs::for('home', fn() => $this->then('home', route('home')));
    }
    
    public function foo(): bool 
    {
        return false;
    }
}

Gives me a hint foo.

Yes, indeed, the route file will become larger by defining breadcrumbs, but I still think it would be a great step. I forked the current package and added the definition to the routes. It looks like this in my working application:

Route::screen('notifications/{id?}', NotificationScreen::class)
    ->name('notifications')
    ->breadcrumbs(fn (Trail $trail) => $trail
        ->parent('platform.index')
        ->push('Notifications')
    );

Route::screen('search/{query}', SearchScreen::class)
    ->name('search')
    ->breadcrumbs(fn (Trail $trail, string $query) => $trail
        ->parent('platform.index')
        ->push('Search')
        ->push($query)
    );

Route::screen('roles/{roles}/edit', RoleEditScreen::class)
    ->name('platform.systems.roles.edit')
    ->breadcrumbs(fn (Trail $trail, Role $role) => $trail
        ->parent('platform.systems.roles')
        ->push('Role', route('platform.systems.roles.edit', $role))
    );

I understand that people may not like this, but are you considering this an additional possibility?

tabuna avatar Apr 22 '20 23:04 tabuna

I'm not overly familiar with IDEs - Sublime Text user here. I wonder if there's a magic comment I can put when calling the closure that would indicate to any IDEs what the context of $this will be?

I'll definitely consider allowing breadcrumbs to be added directly on routes. For now I'm just working on getting the new API working and polishing a few underlying issues I'm having.

dwightwatson avatar Apr 22 '20 23:04 dwightwatson

I'm not overly familiar with IDEs - Sublime Text user here. I wonder if there's a magic comment I can put when calling the closure that would indicate to any IDEs what the context of $this will be?

I didn’t succeed. Such things are already present in Laravel, for example:

Route::macro('example', function ($url) {
    return $this->get($url);  // Here $this is the object of the Route
});

And he also gives false advice.

I'll definitely consider allowing breadcrumbs to be added directly on routes.

I would be happy to help with this.

tabuna avatar Apr 22 '20 23:04 tabuna

Cool, let me nail down some of the core bits and pieces first.

As far as IDE typehinting it's not going to be a priority for me. But if there is an easy way to implement it I don't see any issue with having it.

The problem I'm dealing with now is passing in route parameters. By default I'll pass in all the route parameters directly.

Route::get('/posts/{postId}', 'PostsController@show')->name('posts.index');

Breadcrumb::for('posts.index', function ($postId) {
    //
});

However I might actually want the Post model that the controller loads. (I have use-cases in my app where I specifically don't want to use route model binding, but I still want the model in my breadcrumb definition.

I'm thinking calling Breadcrumbs::render() will use the default route parameters, but you can override it by passing in your own arguments, like Breadcrumbs::render($post). Do you have any thoughts on this?

dwightwatson avatar Apr 23 '20 00:04 dwightwatson

However I might actually want the Post model that the controller loads. (I have use-cases in my app where I specifically don't want to use route model binding, but I still want the model in my breadcrumb definition.

Now the definition of binding in Laravel, works thanks to reflection. Installed in SubstituteBindings middleware. Which calls ImplicitRouteBinding Which in turn is tied to the performer:

//src/Illuminate/Routing/Route.php:475

public function signatureParameters($subClass = null)
{
    return RouteSignatureParameters::fromAction($this->action, $subClass);
}

I did not see the possibility of passing an argument to change the view from route['uses']. It seems that to do this we need to duplicate this method, but pass the closure function for breadcrumbs, and not for the controller.

I'm thinking calling Breadcrumbs::render() will use the default route parameters, but you can override it by passing in your own arguments, like Breadcrumbs::render($post). Do you have any thoughts on this?

I have never had such a need. If I had not read the documentation, then it would have seemed to me that the render method should take the name of the route for which it should build breadcrumbs.

tabuna avatar Apr 23 '20 08:04 tabuna