Skip to content
This repository has been archived by the owner on Oct 18, 2020. It is now read-only.

Added string class support #129

Closed
wants to merge 3 commits into from
Closed

Conversation

xeno010
Copy link

@xeno010 xeno010 commented Aug 27, 2016

Add support for string classes (like routes)

This is the first step for cached Breadcrumbs.

Example: breadcrumbs.php

<?php
/**
* The breadcrumbs.php
**/


Breadcrumbs::register('project-home', function($breadcrumbs, $param)
{
    $breadcrumbs->push('Home - ' . $param, '');
});
//Call add methode in ProjectBreadcrumbController class
Breadcrumbs::register('project-timeline', '\App\Breadcrumbs\ProjectBreadcrumbController@add');
//Call timeline methode in ProjectBreadcrumbController class
Breadcrumbs::register('project-timeline', \App\Breadcrumbs\ProjectBreadcrumbController::class.'@timeline');
//Call __invoke methode in ProjectBreadcrumbController class
Breadcrumbs::register('project-events', \App\Breadcrumbs\ProjectBreadcrumbController::class);

Example: ProjectBreadcrumbController

<?php

namespace App\Breadcrumbs;

use DaveJamesMiller\Breadcrumbs\Generator;

class ProjectBreadcrumbController
{

    public function timeline(Generator $breadcrumbs, $time = '')
    {
        $breadcrumbs->push('Timeline- ' . $time, '');
    }

   public function add(Generator $breadcrumbs)
    {
        $breadcrumbs->push('Add');
    }

    function __invoke(Generator $breadcrumbs)
    {
        $breadcrumbs->push('Events', '');
    }
}

@d13r
Copy link
Owner

d13r commented Aug 28, 2016

This is the first step for cached Breadcrumbs

Have you verified that caching would actually speed it up? As discussed in #112 I have my doubts, but I'd like to see some benchmarks.

@xeno010
Copy link
Author

xeno010 commented Aug 29, 2016

The primary target is a better structure. I have more than 50 definitions in my breadcrumbs.php.
I find "string class" better than "anonymous function" (by many definitions). And this is only a option

@d13r
Copy link
Owner

d13r commented Aug 29, 2016

OK so ignoring the caching/speed issue, I think:

  • The namespace should be optional, the same as in routes
  • Controller is the wrong suffix to use, I'd suggest just Breadcrumbs
  • Maybe they should be in the Http namespace, the same as Controllers
  • If we're going to create a whole class, we should be able to skip the $breadcrumbs argument on each method
  • We might as well skip Breadcrumbs::register() completely by combining it with Add support for custom breadcrumbs action in route #74 - register the breadcrumbs directly against routes

So that would give us something like this, with no breadcrumbs.php required:

// routes/web.php
Route::get('forums/category/{category}', ['as' => 'forums.category',
    'uses' => 'ForumsController@category', 'breadcrumb' => 'ForumsBreadcrumbs@category']);

// app/Http/Breadcrumbs/ForumsBreadcrumbs.php
namespace App\Http\Breadcrumbs;

use App\Models\Category;
use DaveJamesMiller\Breadcrumbs\Breadcrumbs;

class ForumsBreadcrumbs extends Breadcrumbs
{
    public function category(Category $category)
    {
        $this->parent('home'); // Uses the 'home' route
        //OR $this->parent('HomeBreadcrumbs@index');
        $this->push($category->title, route('forums.category', $category->id));
    }
}

What do you think?

@xeno010
Copy link
Author

xeno010 commented Aug 29, 2016

That is a really cool idea.

I have implemented it.

TODO:

  • add Unit-Tests

better namespace handling

fix parseRoutes

remove debug infos

fix unit tests
@xeno010
Copy link
Author

xeno010 commented Aug 29, 2016

Now supported:

<?php
/**
* The breadcrumbs.php
**/
Breadcrumbs::register('AAA', function($breadcrumbs, $param)
{
    $breadcrumbs->push('Home - ' . $param, '');
});

Breadcrumbs::register('BBB', 'BBBBBreadcrumbs@add');

Breadcrumbs::register('CCC', '\Foo\Bar\CCCCBreadcrumbs@add')
# routes.php

Route::get('forums/category/{category}', ['as' => 'forums.category',
    'uses' => 'ForumsController@category', 'breadcrumb' => 'ForumsBreadcrumbs@category']);

Route::get('forums/category/{category}', ['as' => 'forums.category',
    'uses' => 'ForumsController@category', 'breadcrumb' => '\Foo\Bar\ForumsBreadcrumbs@category']);

@d13r
Copy link
Owner

d13r commented Sep 11, 2016

Thanks for this. Just wanted to update you - my current thought is to include this in the next major version, as it's a big change. I may make it the default and deprecate the old way - I need to think about that more and experiment with it on my own projects to see which feels more natural. And the documentation will need updating to explain how to use it. I don't expect I'll have time to do that for several weeks - say late October.

One more thing I want to look into - it seems in Laravel 5.3 the ['as' => 'name'] format is deprecated in favour of the ->name('name') chained method. I'd like to dig around in the code to see if it's possible to add a ->breadcrumb('name') method in the same way... Though on first look I don't think that's possible without replacing the whole Router instance.

@d13r
Copy link
Owner

d13r commented Oct 8, 2016

Laravel Breadcrumbs is no longer officially maintained
Please see the README file for further details.

@d13r d13r closed this Oct 8, 2016
@d13r
Copy link
Owner

d13r commented Oct 8, 2016

Sorry, I've decided I no longer have the time or motivation to continue supporting this package, so I won't be merging this. Thanks for taking the time to implement it though - hopefully someone will fork the package and consider including this.

Another option I thought of is to keep the old Breadcrumbs::register(...) syntax but simply split the breadcrumbs.php file up into multiple files (e.g. breadcrumbs/forums.php, breadcrumbs/pages.php), rather than using classes. That is already supported by overriding the registerBreadcrumbs() method.

@decadence
Copy link

@davejamesmiller could you create last issue where people can decide who will fork and maintain your package?

@d13r
Copy link
Owner

d13r commented Oct 9, 2016

#137

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

Successfully merging this pull request may close these issues.

3 participants