Update defaults for Twitter Bootstrap 3

[levacic]

When this library was created, Twitter Bootstrap was at version 2, so the default divider element / was used. Twitter Bootstrap 3 uses pseudo elements for this, so it's not needed anymore, and should default to null (so it isn't even rendered at all). We should also update the default breadcrumbs class to be just breadcrumb instead (which is the Bootstrap class in both 2 and 3 - not sure why we didn't use that from the start).

In fact, since pseudo-elements are the "correct" way to do this (there's nothing semantic about having a divider in HTML, at least in my opinion), we should likely just remove the divider functionality.

As both of these (removing dividers and changing the default CSS class) are BC breaks, we won't do this before v4.0.0. Not sure when that's gonna happen, but I'll let this issue stay here so we don't forget about it.

If anyone has any input on this (or a good argument on why it shouldn't be done), feel free to comment!

[nelson6e65]

I think this will not going to break too much (or nothing), since if you use this Breadcrums, you MUST solve this manually by removeClass('breadcrums'), addClass(breadcrumb') and also setDivider(null).

There is not need to remove $divider, since (due flexibility of Breadcrums to add and remove classes and divider) you can use it as standalone, using your own style and CSS classes instead of Bootstrap if you like to.

Changes in that commit just default to Bootstrap style.

So, I think a minor release, clarifying this changes in readme and sources, will compliance semver.

[levacic]

That's true in case of users who use Bootstrap - but we can't make that generalization, and for non-Bootstrap users who rely on the default class value, this would break their styling. As a user of any other library, I hate it when a minor update breaks something in my app, and I don't want to do that to other people.

Regarding the BC break - I understand that there are certain assumptions that need to be made about reasonable use cases when using semantic versioning - because strictly speaking, anything you do might be a breaking change for someone - but in this case, I don't think it's unreasonable for people to not be using Bootstrap :)

As for removing the divider, the purpose is to simplify the library, because this can be achieved with CSS alone (as Bootstrap 3 does it, for example), and in my opinion, it's the correct (semantic) way to do it.

[nelson6e65]

You are right. That divider should be added in CSS in any case.

Well... In the mean time, I'll inherit from it in other class and set default state in my constructor.

Hehe... This is a good utility.

~nelson6e65 El 25/04/2016 00:57, "Miloš Levačić" [email protected] escribió:

That's true in case of users who use Bootstrap - but we can't make that generalization, and for non-Bootstrap users who rely on the default class value, this would break their styling. As a user of any other library, I hate it when a minor update breaks something in my app, and I don't want to do that to other people.

Regarding the BC break - I understand that there are certain assumptions that need to be made about reasonable use cases when using semantic versioning - because strictly speaking, anything you do might be a breaking change for someone - but in this case, I don't think it's unreasonable for people to not be using Bootstrap :)

As for removing the divider, the purpose is to simplify the library, because this can be achieved with CSS alone (as Bootstrap 3 does it, for example), and in my opinion, it's the correct (semantic) way to do it.

— You are receiving this because you commented. Reply to this email directly or view it on GitHub

[nelson6e65]

Oh... well, no. Because I was intended to use it from Laravel package. :disappointed:

~nelson6e65

[levacic]

Not really - the creitive/laravel4-breadcrumbs and creitive/laravel5-breadcrumbs packages are there for Laravel integration, but all they really do is configure a shared Breadcrumbs instance (and a facade for people who use that) so that you can access it throughout the app (and take advantage of automatic dependency resolution, etc.)

This package itself (creitive/breadcrumbs) started out as a Laravel-specific package (at version 1), but was refactored to make it standalone starting from version 2 (at that point, the Laravel-specific bridge packages were introduced).

If you are unsure about using the package in a non-Laravel context, feel free to ask and I'll do my best to help out.

[nelson6e65]

Well.. I made this defaults in my base Controller:

<?php

use Breadcrumbs;
use Illuminate\Routing\Controller as BaseController;

abstract class Controller extends BaseController
{
    // protected $breadcrumbs = null;
    public function __construct()
    {
        Breadcrumbs::setCssClasses('breadcrumb')
            ->setDivider(null)
            ->add('<i class="fa fa-fw fa-home"></i>', '/');
    }
}
?>

And spread like that in inherited controllers just adding breadcrumbs.

In my layout:

{!! $breadcrumbs or Breadcrumbs::render() !!}

[levacic]

Sure, that's how you'd use it with Laravel - but if I understood correctly, you don't want to use it with Laravel? I am not sure where the confusion is coming from, so if you have a specific question, please ask and I'll try to help you - additionally, you might want to create a new issue, as this one is specifically about changing the defaults in a future version.

Thanks for using this library and for making it better! :)

[nelson6e65]

No, no. I want to use it in my Laravel project.

Originally I was thinking about to create a ˋBreadcrumb2 extends Breadcrumbsˋ class and set defaults (like I did in example commit) in this inherited class and the use it. But, Laravel package uses ˋBreadcrumbsˋ, not my ˋBreadcrumbs2ˋ. So, it was not going to work. Jaja. So, the las example code was the way to set this defaults in shared $breadcrums from my base Controller. So, now is set to default bootstrap in a Laravel context. :smile:

[levacic]

Ah, that's what you wanted.

You can do that a bit more elegantly if you like. Assuming you're using the creitive/laravel5-breadcrumbs package, you can extend its service provider, and configure that one in config/app.php instead of the one that comes with the package.

Something like this:

<?php

namespace Nelson\Breadcrumbs;

use Creitive\Breadcrumbs\Breadcrumbs;
use Creitive\Breadcrumbs\BreadcrumbsServiceProvider as BaseServiceProvider;

class BreadcrumbsServiceProvider extends BaseServiceProvider
{
    /**
     * {@inheritDoc}
     */
    public function register()
    {
        $this->app['breadcrumbs'] = $this->app->share(function ($app) {
            $breadcrumbs = new Breadcrumbs();

            $breadcrumbs->setCssClasses('breadcrumb');
            $breadcrumbs->setDivider(null);
            $breadcrumbs->add('<i class="fa fa-fw fa-home"></i>', '/');

            return $breadcrumbs;
        });

        $this->app->alias('breadcrumbs', 'Creitive\Breadcrumbs\Breadcrumbs');
    }
}

Now when you use the Breadcrumbs facade, or if you use Laravel's automatic dependency injection, everything will be preconfigured, and you won't have to do it manually in your base controller.

Also, not sure if the add() method should be called in the service provider, this depends on your project I guess - if you have that in all breadcrumbs as the first crumb, I guess it's okay, otherwise, you might want to leave that in your base controller.

[nelson6e65]

Ah, yes. I'll check it this tomorrow if I can implement that instead.

And yes, I added first element because it was the base controller. Anyway, I can reset breadcrumbs in other controller constructor if I need to... Because this plugin is nice! :smile:

Well... I think I'll left controller implementation, waiting this update. o/