Skip to content

Conversation

likeadeckofcards
Copy link
Contributor

Allow user to add another menu to a menu, either as a sub-menu or merged into the current menu. This will be good for when there is various levels of permissions.

Michael Deck added 4 commits May 11, 2017 07:14
Allow the user to add a menu as a sub item of a menu.
Add support for sub menu addition
Allow Menu to be merged with current menu or make a sub menu
Copy link
Member

@TheDeadCode TheDeadCode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should most likely use an IDE like PHPStorm to prevent most style or linting issue highlighted. Regular editors tend to mess up a bit the spaces.

$this->menu->add('Logout')->route('auth.logout'); // Method 6
$this->menu->add('Some Other Link', ['url' => '/link']); // Method 7
$this->menu->add('Sub Menu', 'menu:menu_name); // Method 8
$this->menu->add('Sub Menu', 'sub-menu:sub_menu_name); // Method 9
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot closing single-quote

$this->menu->add('Edit Myself', 'route:users.edit|id:5'); // Method 5
$this->menu->add('Logout')->route('auth.logout'); // Method 6
$this->menu->add('Some Other Link', ['url' => '/link']); // Method 7
$this->menu->add('Sub Menu', 'menu:menu_name); // Method 8
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot closing single-quote

}

return $this;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bad indentation. Make sure you use spaces (4) instead of tabs.

if(starts_with($config, 'menu:')) {
$items =& $this->_items;
$menu = app('menu')->get(str_replace_first('menu:', '', $config));
} else {// if(starts_with($config, 'sub-menu:')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the comment containing logical code


if(starts_with($config, 'menu:')) {
$items =& $this->_items;
$menu = app('menu')->get(str_replace_first('menu:', '', $config));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should either return $this (soft failure) or throw exception if menu doesn't exist.



// Is it another Menu?
if(starts_with($config, 'menu:') || starts_with($config, 'sub-menu:')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space after if

if(starts_with($config, 'menu:') || starts_with($config, 'sub-menu:')) {
$slug = $item['__slug__'] = snake_case($title);

if(starts_with($config, 'menu:')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space after if

$items =& $this->_items;
$menu = app('menu')->get(str_replace_first('menu:', '', $config));
} else {// if(starts_with($config, 'sub-menu:')) {
$items =& $item->_items;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$item->_items is a protected item and it should stay that way. I'd suggest moving the functionality away from the add method and move it to a merge method under the trait CreatesMenuItems.

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

Successfully merging this pull request may close these issues.

3 participants