Build admin menu using configuration #260
Conversation
app/models/spree/admin/main_menu/default_configuration_builder.rb
Outdated
Show resolved
Hide resolved
app/models/spree/admin/main_menu/default_configuration_builder.rb
Outdated
Show resolved
Hide resolved
rafalcymerys
force-pushed
the
feature/create_menu_configuration_object
branch
2 times, most recently
from
823bec7
to
fbb0ba9
Compare
|
|
||
| def insert_before(item_key, item_to_add) | ||
| item_index = index_for_key(item_key) | ||
| raise ArgumentError, "Item not found for key #{item_key}" unless item_index |
There was a problem hiding this comment.
Maybe a KeyError would be more fitting here?
There was a problem hiding this comment.
Also - it seems that this logic (get index by key, raise if the key is nil) is used in multiple places. Maybe we could use a second function like index_for_key! or fetch_index_for_key (mirroring Hash#fetch) which would wrap the exception.
There was a problem hiding this comment.
You mean, adding a private method that would throw an exception if key is not found?
There was a problem hiding this comment.
Yeah, just so the concrete exception is only defined once.
|
|
||
| def add_products_section(root) | ||
| items = [ | ||
| ItemBuilder.new('products', admin_products_path).with_match_path('/products').build, |
There was a problem hiding this comment.
Is there any advantage to using the builder here rather than simply using the Item constructor?
There was a problem hiding this comment.
More explicit interface, getting default values for e.g. translation key, and skipping unnecessary arguments in the constructor.
| Spree::Backend::Config = Spree::Backend::Configuration.new | ||
| app.config.spree_backend = Environment.new |
There was a problem hiding this comment.
- I think "Environment" is not a great name in this context.
- Isn't it a little awkward that now there are two backend "configs"? The Preference-based
Spree::Backend::Configand thisEnvironmentstruct.
There was a problem hiding this comment.
This is a pattern that we use in spree_core as well.
Preferences are persisted in the database, this is slightly different.
rafalcymerys
force-pushed
the
feature/create_menu_configuration_object
branch
from
fbb0ba9
to
3ae2197
Compare
rafalcymerys
force-pushed
the
feature/create_menu_configuration_object
branch
from
3ae2197
to
64d455d
Compare
delete-merged-branch
bot
deleted the
feature/create_menu_configuration_object
branch
A bit of a backstory: A lot of spree extension currently rely on deface gem, which is no longer recommended as it introduces a lot of complexity that's hard to debug, it's prone to errors and its performance is not that great.
As a part of moving away from deface, we need to create interfaces for extensions to modify the admin panel. This is an attempt to move main menu from hardcoded templates, to an object-oriented structure which allows to define and modify the menu using ruby code.
An extension should be then able to modfiy
Rails.application.config.spree_backend.main_menuby either using interfaces ofAdmin::MainMenu::Sectionto mutate/extend the structure, or by providing a totally customized menu.I've also migrated the existing menus to
Admin::MainMenu::DefaultConfigurationBuilder, which can also act as a demo of the tools I've introduced.I would love to hear your feedback on the overall structure.