Laravel-Backpack / CRUD

Build custom admin panels. Fast!
https://backpackforlaravel.com
MIT License
3.04k stars 885 forks source link

[Bug] Multi level menus not consistent across themes & inconsistent behaviour. #5418

Closed piethonkoop closed 6 months ago

piethonkoop commented 7 months ago

Bug report

What I did

After upgrade to v6, changed the menu structure to the newer blade directives + changed to tabler theme

The current menu structure contains 2 submenus under different headings So: A A1 A2 A2.1 A2.2 B B1 B1.1 B1.2 B2

What I expected to happen

menu functionality remains the same across both themes

What happened

with tabler theme, the second submenu (B1 in the example above) does not open although the html is there. Instead, the original menu collapses (so: click on B, submenu B1/B2 opens; click on B1, B closes)

If I try the same with A, it all works. So: Click on A opens A1/A2, click on A2 opens A2.1/A2.2 menu)

switching back to coreuiv2 it all works as expected.

Is it a bug in the latest version of Backpack?

After I run composer update backpack/crud the bug... is it still there?

Yes

Backpack, Laravel, PHP, DB version

When I run php artisan backpack:version the output is:

PHP VERSION:

PHP 8.2.13 (cli) (built: Nov 24 2023 08:46:50) (NTS) Copyright (c) The PHP Group Zend Engine v4.2.13, Copyright (c) Zend Technologies with Zend OPcache v8.2.13, Copyright (c), by Zend Technologies

LARAVEL VERSION:

10.39.0.0

BACKPACK PACKAGE VERSIONS:

backpack/basset: 1.2.2 backpack/crud: 6.5.1 backpack/generators: v4.0.2 backpack/permissionmanager: 7.1.1 backpack/pro: 2.0.20 backpack/settings: 3.1.0 backpack/theme-coreuiv2: 1.2.2 backpack/theme-tabler: 1.2.0

welcome[bot] commented 7 months ago

Hello there! Thanks for opening your first issue on this repo!

Just a heads-up: Here at Backpack we use Github Issues only for tracking bugs. Talk about new features is also acceptable. This helps a lot in keeping our focus on improving Backpack. If you issue is not a bug/feature, please help us out by closing the issue yourself and posting in the appropriate medium (see below). If you're not sure where it fits, it's ok, a community member will probably reply to help you with that.

Backpack communication channels:

Please keep in mind Backpack offers no official / paid support. Whatever help you receive here, on Gitter, Slack or Stackoverflow is thanks to our awesome awesome community members, who give up some of their time to help their peers. If you want to join our community, just start pitching in. We take pride in being a welcoming bunch.

Thank you!

-- Justin Case The Backpack Robot

jcastroa87 commented 7 months ago

HI @piethonkoop can you share with me the menu example, so i can replicate the error on my side.

Thanks.

piethonkoop commented 7 months ago

Hi @jorgetwgroup ,

Code example below. Trimmed down as much as possible of course and matched the titles to the example above

Thanks for your help :)

P.

`

@if ( Illuminate\Support\Facades\Auth::check())

    <x-backpack::menu-item title=" Home" icon="la la-home" :link="backpack_url('dashboard')" />

@if ( Illuminate\Support\Facades\Auth::user()?->is_admin)
    @can('ViewAdminInfo')
        <x-backpack::menu-dropdown title=" Pricing" icon="la la-group">
            <x-backpack::menu-dropdown-item title=" A" icon="las la-euro-sign" :link="backpack_url('a')" />
            @can('ViewNextPrices')
                <x-backpack::menu-dropdown title=" A1" icon="la la-group" nested="true">
                    <x-backpack::menu-dropdown-item title=" A1.1" icon="las la-euro-sign" :link="backpack_url('a11')" />
                    <x-backpack::menu-dropdown-item title=" A1.2" icon="las la-euro-sign" :link="backpack_url('a12')" />
                </x-backpack::menu-dropdown>
            @endcan            
        </x-backpack::menu-dropdown>

    @endcan

    @can('MoveToAdmin')
        <x-backpack::menu-dropdown title=" B" icon="la la-group">
            <x-backpack::menu-dropdown-item title=" B1" icon="las la-caret-right" :link="backpack_url('b1')" />
            <x-backpack::menu-dropdown-item title=" B2" icon="las la-caret-right" :link="backpack_url('b2')" />
            <x-backpack::menu-dropdown title=" B3" icon="la la-group">
                <x-backpack::menu-dropdown-item title=" B3.1" icon="las la-caret-right" :link="backpack_url('b31')" />
                <x-backpack::menu-dropdown-item title=" B3.2" icon="las la-caret-right" :link="backpack_url('b32')" />
            </x-backpack::menu-dropdown>
        </x-backpack::menu-dropdown>
    @endcan
@endif

@endif

`

piethonkoop commented 7 months ago

upon testing with a larger example (more menu dropdowns at highest level), it seems there is more wrong: dropdowns without submenu get closed when switching to different menu, the ones with submenu do not auto close others.

Can't really see why that is happening. Again: only with the tabler theme, not with coreuiv2.

Thanks for your help,

Piet

jcastroa87 commented 7 months ago

Hello @piethonkoop sorry for delay.

I confirm bug, i will assign to our patch team.

Normal menu on "coreuiv2", on "coreuiv4" work ok too

Screenshot 2024-01-12 at 19 01 02

Bug on "tabler", menu didnt show correctly

Screenshot 2024-01-12 at 19 01 58 Screenshot 2024-01-12 at 19 04 41

Cheers.

Antonin-netalis commented 6 months ago

Hi,

I'm also affected by this bug and can't understand why. This is critical, postponing our upgrade to backpack6.

Any news about a patch soon ? Maybe someone else who looked into it has some tips to help fix it on our side while we wait for a fix ?

pxpm commented 6 months ago

Hey @Antonin-netalis we will have a look at this, I couldn't prioritize this when this only affect one theme and in specific scenarios like multi level menus.

We have issues that affect all the users, and those are the ones we put in top priority. As @jcastroa87 pointed out, the issue does not happen in neither of the core-ui themes, so if this is the issue preventing you from upgrade, you can safely upgrade to v6 using coreui-v2 (the same you have in v5) and then if you want to, change the theme to tabler. Actually that is what we recommend in our upgrade guide https://backpackforlaravel.com/docs/6.x/upgrade-guide#step-3.2 , first upgrade using coreui-v2, and then do the theme change, as two separate processes.

I will try to pick this up sometime soon, but without promises, it will depend on my work on other issues (more important in my opinion) than this one. Resources/Time are not infinite, so we need to allocate them properly.

If you find the issue and want to contribute with the fix, feel free to submit a PR, themes and crud are open source packages so any contribution is highly appreciated.

Cheers

Antonin-netalis commented 6 months ago

Hey @Antonin-netalis we will have a look at this, I couldn't prioritize this when this only affect one theme and in specific scenarios like multi level menus.

We have issues that affect all the users, and those are the ones we put in top priority. As @jcastroa87 pointed out, the issue does not happen in neither of the core-ui themes, so if this is the issue preventing you from upgrade, you can safely upgrade to v6 using coreui-v2 (the same you have in v5) and then if you want to, change the theme to tabler. Actually that is what we recommend in our upgrade guide https://backpackforlaravel.com/docs/6.x/upgrade-guide#step-3.2 , first upgrade using coreui-v2, and then do the theme change, as two separate processes.

I will try to pick this up sometime soon, but without promises, it will depend on my work on other issues (more important in my opinion) than this one. Resources/Time are not infinite, so we need to allocate them properly.

If you find the issue and want to contribute with the fix, feel free to submit a PR, themes and crud are open source packages so any contribution is highly appreciated.

Cheers

Hey @pxpm . Thanks for the quick answer. I understand that you have other priorities and that other themes are working fine but we really want to use tabler for different reasons.

Anyway I ended up just rewriting the menu in html without using components since we don't need different layouts. I was afraid at first that the theme was completely affected by the issue and not just the menu components. So not as critical as I thought for us, it will do for now. I couldn't understand what causes the issue but if I do I will gladly submit a PR.

piethonkoop commented 6 months ago

Hi @pxpm

While trying to find a way around this without doing the menus in html, i found another one: if you insert a separator in a submenu, the following happens:

choice of layout (horizontal/vertical) has no impact, both are mangled.

You said:

Actually that is what we recommend in our upgrade guide https://backpackforlaravel.com/docs/6.x/upgrade-guide#step-3.2 , > first upgrade using coreui-v2, and then do the theme change, as two separate processes.

That is exactly what we did.

Short conclusion: the components used for tabler are not production ready.

Given that tabler is the default theme in v6, I ask you to reconsider the priority. Would not want to see people turned away from a further excellent product :)

Thanks

pxpm commented 6 months ago

Thanks for the feedback and for expressing your concerns šŸ™

I think you made a very good point there, I will update this issue in priority so that we tackle this next week. šŸ‘

Cheers

piethonkoop commented 6 months ago

One small other thing while you're at it: if you can define the image both in cover and illustration as a config item that would be great too. It prevents from overriding the blade file. Saves you problems with updates / upgrades..

Thanks

pxpm commented 6 months ago

Hey @piethonkoop I've just fixed the multi level menu issue in tabler 1.2.1. You should be able to get the fix with a composer update.

One small other thing while you're at it: if you can define the image both in cover and illustration as a config item that would be great too.

What blade files are you talking ? Can you give me an example ? Better yet, I will close this issue related with the multi level, and can you please open a separate issue giving more details on this ?

Thanks again for the explanation and sticking with me to fix it šŸ™