datlechin / filament-menu-builder

Create and manage menu in your Filament app.
MIT License
55 stars 11 forks source link

♻ Refactor the menu location implementation #19

Closed Log1x closed 3 months ago

Log1x commented 3 months ago

⚠️ This PR replaces #17 and #18 if merged.

At the moment, multiple menus can be assigned to the same menu location. In my opinion, this goes against the entire point of assigning menu locations. A single menu should be able to be assigned to multiple locations, but there should never be a situation where you need to render multiple separate menus in the same location when you could just handle it with a single menu.

This PR aims to fix that inconsistency bringing the plugin in line with what is seen in CMS such as WordPress with assigning menu locations separately from editing the menu.

To do this, a Manage Locations action has been added to the resource list and edit pages:

Screenshot

With this, the MenuLocations model has been removed and a locations JSON column has been added to the Menu model.

Since I am already breaking everything, I took this opportunity to also rename Menu::menuItems to Menu::items for clarity.

Fetching a menu based on it's assigned location can be done using the Menu::location static method:

use Datlechin\FilamentMenuBuilder\Models\Menu;

$menu = Menu::location('primary');

foreach ($menu->menuItems as $item) {
    //
}

Tóm tắt bởi Sourcery

Tái cấu trúc việc triển khai vị trí menu bằng cách loại bỏ mô hình MenuLocations và giới thiệu một cột JSON trong mô hình Menu để quản lý các vị trí. Thêm hành động 'Quản lý Vị trí' để gán menu tốt hơn, và đổi tên Menu::menuItems thành Menu::items để rõ ràng hơn. Triển khai một phương thức tĩnh để lấy menu theo vị trí.

Tính năng mới:

Cải tiến:

Tài liệu:

Original summary in English ## Summary by Sourcery Refactor the menu location implementation by removing the `MenuLocations` model and introducing a JSON column in the `Menu` model to manage locations. Add a 'Manage Locations' action for better menu assignment, and rename `Menu::menuItems` to `Menu::items` for clarity. Implement a static method to fetch menus by location. New Features: - Introduce a 'Manage Locations' action to the resource list and edit pages, allowing users to assign menus to specific locations. Enhancements: - Refactor the menu location implementation by removing the `MenuLocations` model and adding a `locations` JSON column to the `Menu` model. - Rename `Menu::menuItems` to `Menu::items` for improved clarity. - Add a static method `Menu::location` to fetch a menu based on its assigned location. Documentation: - Update language files to reflect changes in menu location management and visibility options.
Log1x commented 3 months ago

This is ready for review/consideration.

datlechin commented 3 months ago

Great improvements overall.

However, changing existing migrations and model relationship names may introduce breaking changes for current users. Can we minimize potential breaking changes while keeping the enhancements?

Log1x commented 3 months ago

Yeah I will see what I can do. I was mainly squeezing this in early while it was pre-1.0 and minimal installs.

Log1x commented 3 months ago

The MenuLocation model has been added back using the new updated behavior while remaining backwards compatible.

Are you ok with the rename of menuItems to items on the Menu model or do you want me to revert it? Otherwise, all other functionality should work without breaking with my above example still being the ideal usage probably.

In a future PR, I can add a way to override the used Models.

datlechin commented 3 months ago

This change will break my app since I'm using on the menuItems method. We'll need to revert the relationship as well

Log1x commented 3 months ago

No problem. Should be good now.