Automatically hide parent 'yii\bootstrap\Nav' section if all subitems are hidden #119
Conversation
…tion if all subitems are hidden
SilverFire
changed the title
| } | ||
|
|
||
| $items = ArrayHelper::getValue($parentItem, 'items'); | ||
| if (empty($items)) { |
There was a problem hiding this comment.
Could be:
$items = ArrayHelper::getValue($parentItem, 'items', []);
if ($items === []) {There was a problem hiding this comment.
In this case testEmptyItems is failing because we got null and foreach try to run on null.
We can add additional check for null but this is shorter.
|
@yiisoft/core-developers need additional review. |
| @@ -202,6 +202,32 @@ public function renderItem($item) | |||
| } | |||
|
|
|||
| /** | |||
| * Check if parent item is visible | |||
| * @param array $parentItem | |||
| * @return boolean | |||
There was a problem hiding this comment.
Maybe 2.0.6? This extension has no version 2.0.6. released yet and has own version history as I see. Or we need to write framework version here? Why in changelog last changes are written under 2.0.6?
There was a problem hiding this comment.
Yes, 2.0.6, of course. Sorry, I've missed that we are in a separated repo
| * @param array $parentItem | ||
| * @return boolean | ||
| */ | ||
| protected function isParentVisible($parentItem) |
There was a problem hiding this comment.
Name isParentVisible() is not appropriate.
Should be isItemVisible(), to be consistent with isItemActive().
There was a problem hiding this comment.
I think it's more appropriate because only parent items (of the first level) checked like that, and item can be considered as parent even if it don't have any children.
There was a problem hiding this comment.
Name sounds like you are checking if item, which is parent to the given one, is chceked to be visible.
|
It is better to introduce |
|
Since now we have |
|
Maybe add this for now, if related issue is more difficult to implement. The amount of changes is small, so related issue can be implemented separately in the future, |
|
Adding protected method introduces new class API, which future change will cause BC break. |
|
@klimov-paul Could you summarize what's need to be fixed / changed in this case? |
Even if do not extend Actually, all we need to do is 'copy-paste' code from |
Closes #117