Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom classes excluded #41

Closed
mike-sheppard opened this issue Apr 21, 2021 · 8 comments
Closed

Custom classes excluded #41

mike-sheppard opened this issue Apr 21, 2021 · 8 comments

Comments

@mike-sheppard
Copy link
Contributor

Hey @Log1x! 👋🏻

It seems custom classes added to nav items aren't being added to $item->classes.

Screenshot 2021-04-21 at 17 08 00


Had a quick scan through the code and noticed this is excluding everything except what's in the exclusion list? Unless I'm misreading it.

https://github.com/Log1x/navi/blob/master/src/MenuBuilder.php#L90-L95

$classes = array_filter($item->classes, function ($class) {
    return array_key_exists(
        $class,
        array_flip($this->classes)
    );
});

Excluded classes (however they're being allowed). Not sure if this is intentional and simply a typo in the comment:
https://github.com/Log1x/navi/blob/master/src/MenuBuilder.php#L37-L50

/**
  * Blacklisted Classes
  *
  * @var array
  */
protected $classes = [
    'current-menu',
    'current_page',
    'sub-menu',
    'menu-item',
    'menu_item',
    'page-item',
    'page_item',
];

I'll try debugging a bit later on and pop a PR over if I can fix, just thought I'd submit this issue in case you know of a solution off the top of your head.

Cheers!

@jellycode
Copy link

@mike-sheppard I have a pull request in for this issue ( #39 ) already. I couldn't quite figure out what the filter was doing so I just removed it, and everything seems to be working as usual again, but perhaps that isn't the best approach. It got me where I needed to be for the time being at least.

@mike-sheppard
Copy link
Contributor Author

Thanks @jellycode I did see that PR, nice one! I think the idea behind it is to exclude a bunch of the default nav items class bloat, which I'm all for! I'll pop a PR over shortly that preserves this + custom classes :)

@kamilgrzegorczyk
Copy link

@Log1x thank you for your effort!

Is there any plan for releasing that anytime soon?

@Twansparant
Copy link

First of all, thanks for another great package!
I would like to know the same about the classes filter, right now all my custom classes are excluded in the output.
Thanks!

@kamilgrzegorczyk
Copy link

Hey @Twansparant - for the time being I switched my projects to install navi as dev-master Its an ugly solution but does the job for the time being.

@Twansparant
Copy link

Hi @kamilgrzegorczyk, thanks that's what I ended up doing too!

@Log1x
Copy link
Owner

Log1x commented Jun 28, 2021

release pushed – sorry for the delay.

@kamilgrzegorczyk
Copy link

Thank you very much @Log1x !

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

No branches or pull requests

5 participants