Closed tabacitu closed 1 year ago
Fixed by #5061
@maurohmartinez after I've implemented this ui
directory as fallback... I've noticed that in https://github.com/Laravel-Backpack/demo/pull/453 (v6 Demo) we are NOT using the same sidebar_content.blade.php
file for all themes. Quite the contrary, each theme has one of those files, and they are all slightly different. This is a departure from what I first imagined the menu items to be like... something that works across themes. Ideally, all themes would use for the menu items the same blade file. That way, when you switch themes, you keep the same menu items. In order to achieve that, I've went ahead and modified all 3 themes so that sidebar_content.blade.php
loads a new file menu_items.blade.php
. Since that file doesn't exist in any of the themes... but it will exist in resources/views/vendor/backpack/ui/inc/menu_items.blade.php
, basically all themes by default use that menu_items.blade.php
as the source of menu items.
Now... How can we reconcile the differences between the HTML/CSS structure? How can we define menu items in one place... for all themes... and have them look good across themes? Here's what solutions I see:
Basically all themes support the CoreUIv2 syntax for defining menu elements and dropdowns. You tell me if this is possible @maurohmartinez . How different are the syntaxes? Can we do this? How difficult would it be for a new theme developer to do this too?
So instead of having this in menu_items.blade.php
:
<li class="nav-title">First-Party Addons</li>
<li class="nav-item nav-dropdown">
<a class="nav-link nav-dropdown-toggle" href="#"><i class="nav-icon la la-newspaper-o"></i> News</a>
<ul class="nav-dropdown-items">
<li class="nav-item"><a class="nav-link" href="{{ backpack_url('article') }}"><i
class="nav-icon la la-newspaper-o"></i> <span>Articles</span></a></li>
<li class="nav-item"><a class="nav-link" href="{{ backpack_url('category') }}"><i
class="nav-icon la la-list"></i> <span>Categories</span></a></li>
<li class="nav-item"><a class="nav-link" href="{{ backpack_url('tag') }}"><i class="nav-icon la la-tag"></i>
<span>Tags</span></a></li>
</ul>
</li>
We'd have something like:
@backpackMenuSeparator('First-Party Addons')
@backpackMenuItem('<i class="nav-icon la la-newspaper-o"></i> News', backpack_url('article'))
@backpackMenuItem('<i class="nav-icon la la-list"></i> Categories', backpack_url('category'))
@backpackMenuDropdownStart('<i class="nav-icon la la-newspaper-o"></i> News')
@backpackMenuItem('<i class="nav-icon la la-newspaper-o"></i> News', backpack_url('article'))
@backpackMenuItem('<i class="nav-icon la la-list"></i> Categories', backpack_url('category'))
@backpackMenuItem('<i class="nav-icon la la-tag"></i> Tags', backpack_url('tag'))
@backpackMenuDropdownEnd
And then each theme would define how that gets shown (read: what micro-blade-view it loads for each of those helpers, which has the proper classes already).
What do you think @maurohmartinez ?
Also @pxpm and @promatik . What do you guys think? (1) Do you think this new syntax I proposed for menu items... which uses blade directives... is a good or a bad thing? (2) Do you also see it important (as I do) that you define the menu items in one place, and that is even if you change themes? Or do you see each theme as providing their own menu items?
Let's do this! It's one of the last major "quirks" to fix in v6 theming 💪
@maurohmartinez what I would like, rephrased, is to delete all sidebar_content.blade.php
files in our v6 Demo, and have all menu items be stored in one file, for all themes... in resources/views/vendor/backpack/ui/inc/menu_items.blade.php
:
Does that make sense from a developer perspective? Isn't this intuitive and helpful?
Otherwise... when you do php artisan backpack:add-menu-content "some HTML for menu item"
... where would that HTML be placed? In all theme sidebar_content files? Bleah... Right?
We talked about this sometimes in the past and I agree making components agnostic to themes.
Not sure this is a good option tho. I don't think this gives us enough flexibility, I may be wrong, haven't considered all the use cases, but I think we shouldn't make a decision base on the two themes we are supporting.
I think a good strategy to know if we are building a robust and extensible enough theme system is by making sure we can construct any of the sidebars in https://getbootstrap.com/docs/5.0/examples/sidebars/
They are the bare minimum of bootstrap themes. If we can't fully support the "bootstrap example sidebars" I am sure we will have a hard time making it properly work on even more customized themes.
Cheers
Only option B would cover all sidebars... by relegating that to the theme itself. The theme can then output a different HTML, depending on what is used - sidebar, sidebar condensed, topbar, list group, nav etc.
Option A - would make it pretty difficult for a theme developer to add support for all of those. Option B - would make it pretty easy for a theme developer to add support for all of those.
Indeed. Something "along the lines" of option B) I agree with you.
I am just not sure if we can make that option flexible enough to cover all those bootstrap cases, if it does I think it's the best and fair start point we have.
But I think the "golden" point is us (Backpack) don't care about the structure of the sidebar, only the items, and each theme implements their own structure.
So like:
// @sidebarItems($name, $link, $children)
@sidebarItems([
['name', 'link'], ['name', 'link', [
['name', 'link']
]]
])
// or
@item('name', 'link')
@item('name', 'link', [... childs])
@separator()
@item(...)
I agree with option B, without doubts, but I am wrestling with the syntax and the best way to implementing that. Will come back here soon (hopefully) with some ideas.
But I think the "golden" point is us (Backpack) don't care about the structure of the sidebar, only the items, and each theme implements their own structure.
Agreed. But we can't use the simple @item()
and @sidebarItems()
they might conflict with other custom blade helpers, provided by other packages or the app itself.
How about:
@backpackMenuSeparator('First-Party Addons')
@backpackMenuItem('News', backpack_url('article'), 'nav-icon la la-newspaper-o')
@backpackMenuItem('Categories', backpack_url('category'), 'nav-icon la la-list')
@backpackMenuDropdownStart('News', 'nav-icon la la-newspaper-o')
@backpackMenuItem('News', backpack_url('article'), 'nav-icon la la-newspaper-o')
@backpackMenuItem('Categories', backpack_url('category'), 'nav-icon la la-list')
@backpackMenuDropdownEnd
@backpackMenuSeparator('First-Party Addons') @backpackMenuItem('News', backpack_url('article'), 'nav-icon la la-newspaper-o') @backpackMenuItem('Categories', backpack_url('category'), 'nav-icon la la-list') @backpackMenuDropdownStart('News', 'nav-icon la la-newspaper-o') @backpackMenuItem('News', backpack_url('article'), 'nav-icon la la-newspaper-o') @backpackMenuItem('Categories', backpack_url('category'), 'nav-icon la la-list') @backpackMenuDropdownEnd
It sounds better, I am not on naming phase yet.
I am trying to figure out the best strategy for this problem, because we will have more components that would benefit from being theme agnostic.
Sidebar items, are directly composed of:
Indirectly by:
At this point, I am not sure if using @bladeDirectives
is the right approach here.
We could probably get better served by modeling some PHP classes around the sidebar that all the themes could use and exposing a single sidebar api to all the themes.
Themes will receive the SidebarItem
, or SidebarItemGroup
and do whatever they need to display it.
Any theme could easily overwrite those classes to keep the API but change the behaviour.
generators etc will need to have some $property or some getterFunction
where they push to instead of pushing to a blade file.
No no no. If we get to PHP classes we've already over-engineered it. Let's try to find the middle-ground:
Pedro is proposing something more like this:
MenuSeparator::make('First-Party Addons');
MenuItem::make('News')->link(backpack_url('article'))->icon('la la-newspaper-o');
MenuItem::make('Categories')->link(backpack_url('category'))->icon('la la-list');
MenuDropdownStart::make('News', 'la la-newspaper-o')->items(
MenuItem::make('News')->link(backpack_url('article'))->icon('la la-newspaper-o'),
MenuItem::make('Categories')->link(backpack_url('category'))->icon('la la-list'),
);
// because you could also do stuff like:
MenuItem::make('News')->visibleWhen($is_admin); // or
MenuItem::make('News')->hiddenWhen($is_admin);
or
@backpackMenuItem('News', backpack_url('article'), 'nav-icon la la-newspaper-o', $extraAttributes)
Still Pedro - how about named attributes?
@backpackMenuItem(title: 'Categories', icon: 'la la-list', href: backpack_url('category'), visibleWhen: $is_admin, attributes: [
'data-custom-name' => 'smth',
'target' => '_blank',
'class' => 'text-bold',
])
// and in simple cases it would be
@backpackMenuItem(title: 'Categories', icon: 'la la-list', href: backpack_url('category'))
PROs:
CONs:
Since this issue has MOSTLY been resolved, other than ONE sub-item, I've created a new issue for that sub-item, with a summary. Let's continue the conversation in https://github.com/Laravel-Backpack/CRUD/issues/5134 - easier to do that there.
Bug report
There are a few files that should be provided across themes (eg. what is now in
sidebar_content.blade.php
,common.css
etc.)What I did
Switched themes.
What I expected to happen
The menu items to stay the same.
What happened
A different
inc/sidebar_content.blade.php
file was used, so... the menu items changed.What I've already tried to fix it
That is an understandable caveat... but... it's unexpected. It definitely isn't the intention of the developer to lose all menu items when they switch themes so... maybe we can do something about that?
So here's what I'm thinking. What if we use the
UI
term... as the fallback for themes... both for config files and for blade files?config/backpack/ui.php
resources/views/vendor/backpack/ui
That way:
inc/sidebar_content.blade.php
could load aresources/views/vendor/backpack/ui/inc/menu_items.blade.php
menu_items.blade.php
backpack_theme_view()
because that ultimately falls back to the UI directory);