MenuCRUD icon indicating copy to clipboard operation
MenuCRUD copied to clipboard

Revamp MenuCRUD

Open mahasadhu opened this issue 4 years ago • 9 comments

First of all,

this is a major and breaking changes. Please take your time to check thoroughly or just come back later if you got time to inspect, since this is not a major bug or functionality issue fixes.


The way the current MenuCRUD works is to just manage the menu item data. However in the real world website usage, we need more than that, for example multiple menu tree, multiple placement, etc.

I've changed the MenuCRUD functionality to similar to other CMS such as Wordpress styled menu builder. Where we can create multiple menus, assign them placement / location, and easily render them in the frontend / HTML / blade files.

Also in this add on, the menu tree builder (the getTree method), only supports until 2 children / nested tree. I've changed the getTree method to be able to build the tree in basically infinite children. The new function is using reference method to increase the performance rather than recursive method. CMIIW. Source: https://stackoverflow.com/questions/2915748/convert-a-series-of-parent-child-relationships-into-a-hierarchical-tree/34087813#34087813

I've also updated the README.md, hopefully I explain clear enough.

Please also check the service provider, since I've never made a laravel / composer plugin, I'm not sure if I wrote it correctly. 😅 https://github.com/Laravel-Backpack/MenuCRUD/compare/master...mahasadhu:mahasadhu/revamp-menu?expand=1#diff-9c65494514cffe4a90df1778b6da27a28c95a97f6dbb4fb438d056070e4eadef


This PR is still not perfect, for example, we cannot assign a single menu to multiple placement, no active menu detection method, etc. Basically I'm just copy pasting the most basic of what we usually do in our other projects using Backpack.

I'm open to suggestion, just tell me what you guys think, either this PR needs more features / functionalities, too complex, or maybe this should be in a separate add on.

Thanks for checking 🍻 😃

mahasadhu avatar Dec 30 '20 07:12 mahasadhu

BOOM! Your first PR with us, thank you so much! Someone will take a look at it shortly.

Please keep in mind that:

  • if this constitutes a breaking change, it might take quite a while for this to get merged; we try to emulate the Laravel release cycle as much as possible, so developers can upgrade both software once; this means a new big release every ~6 months;
  • even if it's a non-breaking change, it might take a few days/weeks for the PR to get merged; unless it's a no-brainer, we like to have some community feedback on new features, before we merge them; this leads to higher-quality code, in the end; we learnt this the hard way :-)
  • not all PRs get merged; sometimes we just have to hold out new features, to keep the packages lean; sometimes we don't include features that only apply to niche use cases;
  • we're not perfect; if you think we're wrong, call us out on it; but in a kind way :-) we all make mistakes, best we learn from them and build better software together;

Thank you!

-- Justin Case The Backpack Robot

welcome[bot] avatar Dec 30 '20 07:12 welcome[bot]

Nice work here @mahasadhu !

I agree - this is quite a big change in scope... so we should be careful about merging this. Happy to take a look and help guide this into something a lot of devs will use - either inside MenuCRUD or inside its own package, not really sure at this point.

I also agree that MenuCRUD is quite limited in terms of functionality. It only lets you edit one menu. Which is not enough in a lot of websites / web apps. Its purpose from the start wasn't to offer a general-purpose solution, but to scratch a personal itch and to provide a way to show off and test the Reorder functionality in Backpack. I've always thought that maybe we create a package that does more, something like MultipleMenuCRUD. That way:

  • people who need just one menu install MenuCRUD
  • people who need multiple menus install MultipleMenuCRUD

Otherwise, if you only have one menu and install an add-on that has multiple menus but you can't use them... it would make for a bad UX for the admin I think...

How's the interface? Can you show us a few quick screenshots of how it would look&feel from the admin's point of view?

Cheers!

tabacitu avatar Jan 04 '21 13:01 tabacitu

@tabacitu - Thank you for checking, here's the screen record for the interface, hope you guys understand my mouse pointer gesture 😅

https://user-images.githubusercontent.com/13914485/103606383-31b7b380-4f51-11eb-9e27-332e0ab7f5ad.mp4

mahasadhu avatar Jan 05 '21 04:01 mahasadhu

And here is the example for the current active page, I just added a "(current)" string to the current page's menu label

https://user-images.githubusercontent.com/13914485/103606457-6a578d00-4f51-11eb-9b48-6b46e085765c.mp4

mahasadhu avatar Jan 05 '21 04:01 mahasadhu

I like it a lot @mahasadhu!

I agree with @tabacitu, most of the people use a single menu, and having an UI for multiple is kinda bad UX. But, I don't like the idea of having 2 packages for the Menu, it would be nice if developers installed only one package and could "opt-in" for multiple menus or not. (I don't know how to do it 😅, just saying).

Thank you for the PR and for the videos, we'll take a better look at this on v4.2, I'm adding this there for now ✌

promatik avatar Feb 20 '21 11:02 promatik

hi @promatik , thank you for checking

I see, hmm using my current PR, I think I'll just need to insert a default Menu in the migration file. Using that default menu, backpack user which only need one Menu can just manage the Menu Items in that default Menu.

So when the single_menu option in config is true, backpack user can only access the Menu Items CRUD in that default Menu. Otherwise, user can edit both Menu and Menu Items

This Menu and Menu Items is quite mouthful, I hope you understand what I'm trying to explain 🤣

mahasadhu avatar Feb 22 '21 02:02 mahasadhu

By the way is there any way to make Backpack operations optional? Using if maybe?

mahasadhu avatar Feb 22 '21 02:02 mahasadhu

@mahasadhu I think there is no way to make an Operation optional. You always load the Operation. Inside the operation you may have conditions to load your content or not;

In my case I had an Operation for creating dummy entries, where it added a button to the list view only if the model supports factories, so the condition is on the setup; https://github.com/promatik/create-dummy-operation-for-backpack/blob/master/src/Http/Controllers/Operations/CreateDummyOperation.php#L37

promatik avatar Feb 22 '21 10:02 promatik

@promatik ah I see, the good ol reflection way 😆

Thank you for the insight, will come back later when I managed to make the multi menu as opt-in

By the way @promatik could you please check this? https://github.com/Laravel-Backpack/CRUD/issues/3457

mahasadhu avatar Feb 23 '21 03:02 mahasadhu

Hey @mahasadhu ,

Unfortunately we've decided not to do this in the current package. It's a big change, basically adding another layer - you don't have ONE menu, but MULTIPLE menus. Which is something most projects won't need. So it adds complexity for like 80% of the projects... that they wouldn't need.

I do have projects that COULD use this, and SHOULD use this though. So I'd definitely use this package if it existed. What do you think about creating an add-on for this, separate from MenuCRUD? Something like mahasadhu/multiple-menu-crud? It's pretty easy to do that using our tutorial here, and if you do create and maintain it, we could promote it to the Backpack community.

Let us know if you want to build it. If not, maybe someone else in the community will do it. Sorry for the bad news. Cheers!

tabacitu avatar Jun 06 '23 17:06 tabacitu