Passing active status to Dropdown from Nav/isItemActive to fix #133

[YiiRocks]

Q	A
Is bugfix?	yes
New feature?	no
Breaks BC?	yes
Tests pass?	yes
Fixed issues	#133

[YiiRocks]

This code works:

echo yii\bootstrap4\Nav::widget([
        'items' => [
            ['label' => 'Main', 'url' => ['/site/index']],
            [
                'label' => 'Admin',
                'items' => [
                    ['label' => 'Users', 'url' => ['/site/users'], 'active' => true],
                    ['label' => 'Roles', 'url' => ['/site/roles']],
                    ['label' => 'Statuses', 'url' => ['/site/statuses']],
                ],
            ],
        ],
]);

Removing active and let the route detection does its magic gives me nothing... Any help would be appreciated.

[samdark]

@simialbi, @AsDonAs any ideas?

[YiiRocks]

@samdark: Tests pass now, but I don't understand.

I changed the test condition itself and suddenly get different result. AND without changing the src it affected another test result. Is something wrong with the NavTest file?

Edit: using my repository on my website now as a real-life test and actually got items active :+1:

[simialbi]

I really think this pr produces more problems than it solves. I watched the code and I think e.g. it will break the active state of a Nav inside a NavBar. To be honest, I would revert #139 too and look for a better solution.

[YiiRocks]

NavBar itself never calls on Nav, so if it works from Nav and from DropDown (like it does now), it works. What problems do you see this PR creating? It is just passing 1 value ((bool) $active) down to DropDown, nothing more.

Revert of #139 is also taken care of in this PR, as the option was made obsolete and therefor deleted.

[simialbi]

Pls see https://github.com/Thoulah/yii2-bootstrap4/pull/1, I think this is absolutely sufficient. What do you think?

[YiiRocks]

There are 4 failing tests Also I would like to have some consistency. We have active option in Nav and therefor I prefered to copy the same behaviour in Dropdown. This seems prefered over setting 'active' => true in one module and 'linkOptions' => ['class' => 'active'] in another.

[simialbi]

Fixed https://travis-ci.com/Thoulah/yii2-bootstrap4/builds/114594113 I understand your intention. But the use case of setting a dropdown item manually active, outside a navigation tends nearby 0. And inside a navigation the navigation class sets the active state. In my opinion it's beautiful but not necessary and a bit overkill.

[YiiRocks]

Perhaps setting it manually to true or false tends to nearby 0, but from a condition is not a rare occasion (eg 'active' => date('w') === 5). For the seconds part, why not create clean code without useless clutter.

I only added 1 variable and 1 condition, so I don't see how your solution is much cleaner.

On the positive side, I like you chances to the tests very much and will happily take those over (in the evening, at work now), along with credits to you for your time spend on this issue :+1: I will make a manual merge of both your changes and mine. I expect it to be very clean, with nice tests.

[YiiRocks]

I think it's now very clean. If you see any possible pitfalls, please let me know.

[YiiRocks]

That's on purpose. See https://getbootstrap.com/docs/4.3/components/navs/

[simialbi]

It's not in Navbar: https://getbootstrap.com/docs/4.3/components/navbar/#nav

[YiiRocks]

Ok, did not see that, but:

Active states—with .active—to indicate the current page can be applied directly to .nav-links or their immediate parent .nav-items.

Since we have them applied on nav-links already now, nav-items can be left as is.

[samdark]

@simialbi, @Thoulah please ping me when the pull request is ready for final review/merge.

[YiiRocks]

@simialbi: Was ist deine Meinung?

[simialbi]

@Thoulah: Oh bitte Duze mich. So alt bin ich noch nicht 😄 .

Active states—with .active—to indicate the current page can be applied directly to .nav-links or their immediate parent .nav-items.

Das war mir nicht bekannt. Ich bin mir aber ziemlich sicher, dass das nicht immer so war. Somit müsste man evtl. im composer.json noch den minimalen Version Constraint von npm-asset/bootstrap auf die aktuelle Version ^4.3 updaten (um sicher zu gehen).

[YiiRocks]

Sorry, Worte kenn ich, aber die Grammatik... ärgerlich Vielleicht wurde Boostrap geändert, das weiß ich nicht. Ich werde die Minimalversion ändern und fertig :+1:

@samdark: Мы готовы

[simialbi]

Oh I'm sorry, I thought cause you're location is defined as Germany. For me it's ok like this. I think it could be merged. @samdark I think we're ready

[YiiRocks]

It is Germany, but not native German

[samdark]

Merged. Thank you!