Skip to content

[Feature] Add ability to create Menu groups#384

Closed
mallardduck wants to merge 11 commits intofilamentphp:developfrom
mallardduck:feature-navigation-groups
Closed

[Feature] Add ability to create Menu groups#384
mallardduck wants to merge 11 commits intofilamentphp:developfrom
mallardduck:feature-navigation-groups

Conversation

@mallardduck
Copy link
Copy Markdown
Contributor

@mallardduck mallardduck commented Apr 21, 2021

Feature Summary

This PR adds a comprehensive API that allows grouping of Resource and Page elements in the Navigation.
The method to do so range from very simple single line of code additions, to an enhanced version of that with Mapped groups, or simply making a Page or Resource itself the top level group element.

Effectivly this feature provides this in two flavors:

  • Simple Groups: Those that are grouped into a submenu with no top-level page.
  • Dynamic Groups: Those that are grouped into a submenu based on a Page or Resource that implements IsGroupItem.

Simple Groups:

Any resources that defines a $navigationGroup property will be grouped by overlapping names.
This should be the menu label text for the group - with proper spaces and capitalization you'd like.

class BlogPostResource extends Resource
{
    public static $navigationGroup = 'Content Group';
class ContentPage extends Page
{
    public static $navigationGroup = 'Content Group';

So in this example there would be a new Content Group sub menu with both these in it.

Simple Groups with Mapped Resources

The second method mimics how Laravel allows for Eloquent morphMap to be used. In eloquent this maps long class names into uniform database values - for us it maps non-Page and non-Resource groups into a single location.

This method should be seen as an enhancement of the prior one. Since ultimately this is required to control the sort and icon settings for any Simple Groups. This would be something you could add in your AppServiceProvider::register like this:

        Filament::navigationGroupMap([
            [
                'name' => 'Content',
                'sort' => -1,
                'menus' => [
                    'default',
                    'foobar',
                ]
            ],
        ]);

Note: This could be something most useful for the Filament plugins. For instance when a plugin might want to register a group. In theory this should allow all plugins to use the navigationGroupMap without stepping on each others toes.

Dynamic Groups for Resources:

There are two very simple changes one would need to make a Resource into a Group. Simply adding the IsGroupItem interface and the ImplicitGroupFromResource trait. And that should be all they need to do. Like:

use Filament\Resources\Concerns\ImplicitGroupFromResource;
use Filament\View\Concerns\IsGroupItem;

class BlogPostResource extends Resource implements IsGroupItem
{
    use ImplicitGroupFromResource;

And with just those added the Resource will now be a top-level Group item.
Then to add another resource to this group do:

class BlogCategoryResource extends Resource
{
    public static $navigationGroup = 'Blog Posts';

Dynamic Groups for Pages:

There are two very simple changes one would need to make a Resource into a Group. Simply adding the IsGroupItem interface and the ImplicitGroupFromResource trait. And that should be all they need to do. Like:

use Filament\Pages\Concerns\ImplicitGroupFromPage;
use Filament\View\Concerns\IsGroupItem;

class ContentPage extends Page implements IsGroupItem
{
    use ImplicitGroupFromPage;

And with just those added the Resource will now be a top-level Group item.
Then to add another resource to this group do:

class BlogTagResource extends Resource
{
    public static $navigationGroup = 'Content Page';

Results

Simple Groups:

Closed/Open Open/Closed
Screen Shot 2021-04-23 at 12 08 15 AM Screen Shot 2021-04-23 at 12 08 19 AM

Item within SubMenu Selected
Screen Shot 2021-04-20 at 8 59 37 PM

Dynamic Groups:

SubMenu # 1: Blog Posts SubMenu # 2: Content page
Screen Shot 2021-04-22 at 11 07 48 PM Screen Shot 2021-04-22 at 11 07 44 PM

@danharrin
Copy link
Copy Markdown
Member

@adam-code-labx how similar is this to your sub resources pattern?

@mallardduck
Copy link
Copy Markdown
Contributor Author

mallardduck commented Apr 21, 2021

I'm curious to see how an alternative "sub resource" concept would look too.

IMHO between this PR and #320 - the filament system would have a pretty robust way to manage sub-resource forms and the navigation grouping. Granted I made these PRs, but I think one strength of this pattern for Navigation groups is that it's very similar to what I've seen how Nova does things.

While copying nova isn't necessarily the goal here -behind the way I've implemented this- I do think that providing similarly simple APIs would reduce friction for new users. Especially for users already familiar with Nova hoping to try something open-source.

@adam-code-labx
Copy link
Copy Markdown

Hey @danharrin
I plan to work more on the sub resources PR this weekend.

@mallardduck this is good approach for grouping the navigation and could work well for the plugin architecture.

My suggestion for the Filament core "Content" in your example should be a resource, perhaps with new trait IsGroupResource which will allow you to set any resource as the parent resource. instead of setting navigationGroupMap you can set everything in the usual navigation properties and set the extra property to define navigationGroup.

Within this parent resource class you can setup all the logic for blog etc and then create the posts, categories, tags resources too extend this parent class to share functionality across the resources in the same group. Another idea is to perhaps use this parent resource as a mini overview displaying stats and summary of the resources in this group.

Also for the navigation improvements If you check out my last PR I have already started to isolate the Navigation away from the Livewire component so this can simply be used from any component or injected directly in controller and used for tests etc. Also useful if we we need to set multiple navigations.

https://github.com/laravel-filament/filament/blob/587c5a5699ab58415ad4b6cebb5488ac84e0a85f/src/Services/NavigationService.php
I plan to extend on this more in my next PR

This also cleans up NavigationMenu component.
https://github.com/laravel-filament/filament/blob/587c5a5699ab58415ad4b6cebb5488ac84e0a85f/packages/jetstream-extension/src/Http/Livewire/NavigationMenu.php

I am a little limited on time at the moment, but I will see if I can put a few extra hours in this weekend to finish at least one of these PR's 😅

@mallardduck
Copy link
Copy Markdown
Contributor Author

I may need to re-read that to fully understand what you're suggesting. However the "Content" group example here is just that purely a navigational group. Nothing about the Content navigation item has/owns a resource itself nor was it intended to.

However the system I've laid out could be extended to allow the top level group to be a link. To facilitate the group page for stats and overview of the group you've discussed. If that's the desired behavior then it could totally support that.

Personally though, I've started avoiding that UX pattern in my menu's as it adds unnecessary complexity for non-sighted users. For those users it's often much better for the button to simply open/close the submenu and then have an "Overview" item as the first one in the group.

I guess TBH I'm also not following how the jetstream package will tie into overall navigation. Unless do you just mean that code will be refactored into the core package? Well either way just let me know if I need to make changes to this and I'll be happy to make changes. Or if it just won't be useful with w/e plans you guys seem to have feel free to close it and reuse any thing that is still helpful.

@mallardduck
Copy link
Copy Markdown
Contributor Author

instead of setting navigationGroupMap

I think you misunderstand how this all works if you think that setting navigationGroupMap is required. Doing so is only an optional enhancement much alike how the Eloquent::morphMap can enhance the morph relationships.

"Content" in your example should be a resource, perhaps with new trait IsGroupResource
I feel like this is abusing the Resource type since the "Content" label is just as visual label. So it's just for a group of nav links, not a resource with need to render a form or field.

While I do think there's a benefit of letting a resource be a Group as well as a resource - I do not think that is required for all use cases. With this PR users technically only have to add a single line of code to a resource to group them. Setting that will group everything under a submenu.

If all groups had to be Resources as you suggest then -even those that have no model- would have and need to hack around a lot of core concepts of Resource. So the Resource would need to behave very differently when it has no model, needs no form nor any table.


However I think that the suggestion to allow a Resource to implement that interface would be a great way to support both concepts at the same time. Obviously it will need to change for w/e other changes you're planning to make, but this code I added here:

               if ($resource::hasNavigationGroup()) {
                    NavigationGroup::group($resource::$navigationGroup)->push(...$resource::navigationItems());
                } else {
                    $this->items->push(...$resource::navigationItems());
                }

Could end up looking like:

               if ($resource::hasNavigationGroup()) {
                    if ($resource instanceof IsResourceGroup) {
                        NavigationGroup::make($resource::$navigationGroup, null, $resource::$navigationGroupIcon, $resource::$navigationSort)
                    } else {
                        NavigationGroup::group($resource::$navigationGroup)->push(...$resource::navigationItems());
                    }
                } else {
                    $this->items->push(...$resource::navigationItems());
                }

@adam-code-labx
Copy link
Copy Markdown

@mallardduck This sounds good to me.
By giving the option to create a group resource or register the group only for the navigation.
This gives the flexibility for both use cases.

@danharrin Can I suggest if you can please approve this PR before I work further to extend sub resources, which I am hoping to get some time to work on this Sunday. Thanks.

@mallardduck
Copy link
Copy Markdown
Contributor Author

@adam-code-labx - Legit, I've got some changes in the pipeline I'm working on now.
These should provide an API for multiple menus to be managed and for groups to be registered to multiple menus.
I'll also provide the Interface/Trait for a Resource to assume the role of a group.

@mallardduck
Copy link
Copy Markdown
Contributor Author

As I've been working on these changes for supporting the concept of multiple menus...I'm wondering if we should split that into a second effort @danharrin? I.e. I will simply push the commits support for Pages to be sorted into groups so they are symmetrical.

This would not include:

  • Multiple menu registration support for groups.
  • Traits/interfaces for a Resource to register itself as a group via properties.

However both of those could be added in after @adam-code-labx has been able to wrap up and send over their PRs too. Unless having these changes in right now would help your efforts, Adam? In that case I could still shoot it all over as updates on this PR.

@adam-code-labx
Copy link
Copy Markdown

@mallardduck Yeah would prefer to wait until your PR is completed and merged.
Then I will see what additional features I can contribute on this one.

@mallardduck
Copy link
Copy Markdown
Contributor Author

@danharrin & @adam-code-labx - I think this is ready for a look over and merge when deemed acceptable.

@mallardduck mallardduck changed the title [Feature] Add ability to register resources with navigation groups [Feature] Add ability to create Menu groups Apr 23, 2021
@mallardduck
Copy link
Copy Markdown
Contributor Author

Also - I've prepared this PR for the docs: https://github.com/laravel-filament/docs/pull/11

@danharrin danharrin added enhancement New feature or request pending review labels Apr 23, 2021
@danharrin
Copy link
Copy Markdown
Member

Hey @ryanscherler, could I have a design review on this please?

@danharrin danharrin requested a review from ryanscherler April 23, 2021 22:13
@ryanscherler
Copy link
Copy Markdown
Contributor

I like this idea a lot - one consideration would be to not have the left borders that seem to be alternating in the menu items?
Group

@ryanscherler
Copy link
Copy Markdown
Contributor

Another consideration is not having a background for both the group and the selected item, keep the text white for the group, and then a background on the actual selected item. Keeps things more simple design-wise.

@mallardduck
Copy link
Copy Markdown
Contributor Author

mallardduck commented Apr 25, 2021

I like this idea a lot - one consideration would be to not have the left borders that seem to be alternating in the menu items?
Group

They are not alternating - well I mean in this example they are - however that's an affect of the fact that the middle page is selected. I thought the left boarder color state should also be modified along with the main button color to indicate it being selected.

Here's a full page screenshot to help clarify what you're seeing there:

Screen Shot 2021-04-25 at 4 34 51 PM
Screen Shot 2021-04-25 at 4 35 40 PM

@mallardduck
Copy link
Copy Markdown
Contributor Author

mallardduck commented Apr 25, 2021

Another consideration is not having a background for both the group and the selected item, keep the text white for the group, and then a background on the actual selected item. Keeps things more simple design-wise.

While not having a state based background for the group could be an option, it would mean that when a group is closed you loose sight of what group the page you're on is within. That may not be a deal breaker, but it does make things less intuitive to new users.

For instance these screenshots may show the value in having a group level state too. Even if it's different from the existing styles - hopefully this helps show why some sort of state will be helpful:
Screen Shot 2021-04-25 at 4 35 32 PM
Screen Shot 2021-04-25 at 4 35 21 PM

When closed it could be ambiguous what group/sub-menu the active page is under. As you could imagine with more groups and page to be linked in the nav this concern could become an issue. Especially since it also puts an onus on the user to know what group pages/links exist under. By doing that it -IMHO- creates a tough situation is and when a page/link moves groups.

Since the menu's design wouldn't make that explicit they'd rely more on memory, so any change to what's in their memory needs to be done with more caution. I'm all for simple and totally interested in providing a great UX out of the box. I just hope the end solution is one that preserves some of the implicit state that my initial designs provide.

I think this PR is open to reviewer modifications if you'd like to play around with it and refine that aspect. It may help since it's hard to explain the choices I made based on the varying menu configuration and state based styles to cover all cases well. Or if you want to shoot some ideas back and forth that's cool too - whatever works.

@ryanscherler
Copy link
Copy Markdown
Contributor

ryanscherler commented Apr 26, 2021 via email

@mallardduck
Copy link
Copy Markdown
Contributor Author

I think that just indenting the sub group items might be enough - the borders just seem odd to me? Better to go simple if we can?

When this feature was hacked together initially I did not have the border at all and only used an indent.
It just didn't feel obvious to me at first glance so I quickly added the border to it.

Hmm. I wonder if we should make this aspect of the rendering configurable to some extent? Maybe that's a consideration for later though since it doesn't seem that crucial. I'll remove the border and set a decent sized indent - i'm guessing 0.5 rem or 1rem would look best, likely the latter.

@mallardduck
Copy link
Copy Markdown
Contributor Author

@ryanscherler - Here's updated previews:

one two three
Screen Shot 2021-04-26 at 9 18 55 PM Screen Shot 2021-04-26 at 9 18 48 PM Screen Shot 2021-04-26 at 9 18 40 PM
four five
Screen Shot 2021-04-26 at 9 21 32 PM Screen Shot 2021-04-26 at 9 21 39 PM

@ryanscherler
Copy link
Copy Markdown
Contributor

@danharrin I love it! This is exactly what I was thinking.

@danharrin
Copy link
Copy Markdown
Member

danharrin commented Apr 27, 2021

Hey, thanks for your work on this! I've had a chat with @ryangjchandler and we feel the best path forward is to hold off merging this feature until the next major version. We would really like to rebuild our navigation API and include this concept from the core, but doing so would bring breaking changes with it. Thanks again! I will close this PR for now and revisit it in the next few months, when it's ready to be merged.

@adam-code-labx
Copy link
Copy Markdown

Hey @danharrin sorry not had time to add anything yet for the sub resources feature.
Really busy right now with client work.

Can I make a suggestion instead of closing this PR how about creating new branch for example "v2-review" then merging this PR. Once I get around to completing my work on sub resources. I will also merge into this new branch so you guys can review this later for future releases 😏

@ryangjchandler
Copy link
Copy Markdown
Contributor

Hey @danharrin sorry not had time to add anything yet for the sub resources feature.

Really busy right now with client work.

Can I make a suggestion instead of closing this PR how about creating new branch for example "v2-review" then merging this PR. Once I get around to completing my work on sub resources. I will also merge into this new branch so you guys can review this later for future releases 😏

I think this PR would look very different in a v2 due to the changes we want to make to the Navigation API, so it's not really worth opening a v2 branch quite yet.

Of course, the PR will still be here (closed) so we can come back later on & revive it.

@yob-yob
Copy link
Copy Markdown
Contributor

yob-yob commented Oct 4, 2021

well can't wait for Admin v2... I'm really doing some hacky stuff just to accomplish this on v1...

@danharrin
Copy link
Copy Markdown
Member

danharrin commented Nov 7, 2021

Hey guys, just got onto making this feature for v2.

At the moment, I've gone with the ability to set a $navigationGroup (or getNavigationGroup()) for pages and resources.

Then, to adjust the order of these groups, you may registerNavigationGroups() manually:

Filament::registerNavigationGroups([
    'Blog',
    'Content',
]);

@Larsklopstra created an excellent prototype design for this feature, but there is not collapse functionality. This may be added at a later date:

Screenshot 2021-11-07 at 20 08 47

Thoughts? :)

@adam-code-labx
Copy link
Copy Markdown

Hi @danharrin this looks great!
Nice and simple approach with clean design, it would be great to see dark mode as well as a horizontal menu layout option. I guess this can come after you have released v2 admin.

I will be in touched with you towards the end of the week to discuss one of my own projects I am working on and how I plan to utilise filament v2 throughout this project. I would love to get you onboard as one of our 1st few partners for FREE of course. Catch up soon 😁

@ManojKiranA
Copy link
Copy Markdown
Contributor

any update on this to add on up comming filament version

@danharrin
Copy link
Copy Markdown
Member

any update on this to add on up comming filament version

https://filamentadmin.com/docs/2.x/admin/navigation#grouping-navigation-items

@ManojKiranA
Copy link
Copy Markdown
Contributor

I am developing the Inhouse server inventory application. There is huge list of menus that needs to added. So the sidebar becomes very lenghty. It would be nice to add expand/collapse types of menu so that it will be minimal list.

image

@SebastiaanKloos
Copy link
Copy Markdown
Contributor

Also for creating settings pages it would be cool to group them in a Settings group and make it collapsible

@danharrin
Copy link
Copy Markdown
Member

Feel free to create a PR to make these collapsible. Not sure they should be by default though.

@SebastiaanKloos
Copy link
Copy Markdown
Contributor

Feel free to create a PR to make these collapsible. Not sure they should be by default though.

Yeah I think it should be optional indeed. When I have some free time, I can try and make them collapsible

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

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants