sonata-project / SonataAdminBundle

The missing Symfony Admin Generator
https://docs.sonata-project.org/projects/SonataAdminBundle
MIT License
2.11k stars 1.26k forks source link

Impossible to set an icon for "on_top" element menu #4023

Closed maximevenin closed 7 years ago

maximevenin commented 8 years ago

Before making a PR I wanted to make sure I'm not missing something: when setting the option 'on_top' to true on an element of the menu, it is impossible to set an icon for this element:

Sonata packages

$ composer show sonata-project/*
sonata-project/admin-bundle              3.4.0                    The missing Symfony Admin Generator
sonata-project/block-bundle              3.1.1                    Symfony SonataBlockBundle
sonata-project/cache                     1.0.7                    Cache library
sonata-project/core-bundle               3.0.3                    Symfony SonataCoreBundle
sonata-project/datagrid-bundle           2.2                      Symfony SonataDatagridBundle
sonata-project/doctrine-extensions       1.0.2                    Doctrine2 behavioral extensions
sonata-project/doctrine-orm-admin-bundle 3.0.5                    Symfony Sonata / Integrate Doctrine ORM into the SonataAdminBundle
sonata-project/easy-extends-bundle       2.1.10                   Symfony SonataEasyExtendsBundle
sonata-project/exporter                  1.5.0                    Lightweight Exporter library
sonata-project/google-authenticator      1.0.2                    Library to integrate Google Authenticator into a PHP project
sonata-project/user-bundle               dev-rc_sf3tweaks bf121be Symfony SonataUserBundle

Steps to reproduce

Fresh install of SonataAdmin, add the following config:

sonata_admin:
  dashboard:
      groups:
          home:
            on_top: true
            icon: '<i class="fa fa-home"></i>'
            items:
              - route: sonata_admin_dashboard
                label: home

Expected results

A nice "home" icon next to the element menu "home" in the left menu.

Actual results

"Home" is there, but the icon is not (it is not a FontAwesome problem, the html is empty).

Explanation

In sonata-project/admin-bundle/Resources/views/Menu/sonata_menu.html.twig:

{% set icon = item.attribute('icon')|default(item.level > 1 ? '<i class="fa fa-angle-double-right"></i>' : '') %}

The html code for the icon is in this icon variable, but item.attribute() doesn't work.

Either my config is wrong or this line should be:

{% set icon = item.extra('icon')|default(item.level > 1 ? '<i class="fa fa-angle-double-right"></i>' : '') %}
soullivaneuh commented 8 years ago

Please first follow GitHub issue template for bug report.

greg0ire commented 8 years ago

Also, can you link to and explain the relevant piece of php code?

greg0ire commented 8 years ago

I moved your last message into the first one.

ioweb-gr commented 8 years ago

I just bumped into this issue while trying to achieve pretty much the same thing as @Ximousse but for me it's also not giving the correct route as well.

sonata_admin:
  dashboard:
      groups:
          home:
            on_top: true
            icon: '<i class="fa fa-home"></i>'
            items:
              - route: sonata_admin_dashboard
                label: home

produces an <a href="sonata_admin_dashboard">Home</a> link instead of /admin/dashboard in my case and the home icon doesn't show either.

removing on_top: true yields the correct link

@Ximousse is the link also wrong for you besides the icon?

maximevenin commented 8 years ago

Yes, I wanted to create a separate issue for this but indeed the link is wrong (same as you).

ioweb-gr commented 8 years ago

Thank you @Ximousse for confirming, I tracked it down to vendor/sonata-project/admin-bundle/Menu/Provider/GroupMenuProvider.php

line 115 $menuItem->setUri($item['route']);

Seems the conditional if here if (isset($item['admin']) && !empty($item['admin'])) { just sets the route as is in case the route is set in on_top = true groups where admin is not set

I don't know if it's by choice for some reason but I'd think that any route specified in config files should be set as a named route for consistency unless defined by another flag as relative or absolute path.

For my specific use case I changed $item['route'] to $this->pool->getContainer()->get('router')->generate($item['route']); and used your icon fix too temporarily and everything seems fine.

greg0ire commented 8 years ago

$menuItem->setUri($item['route']); definitely looks strange, and was introduced in #3159 by @tim96 . @tim96, can you please help?

tim96 commented 8 years ago

$menuItem->setUri($item['route']); definitely looks strange, and was introduced in #3159 by @tim96 . @tim96, can you please help?

@greg0ire no, I didn't remember. I created it PR a year ago, sorry.

greg0ire commented 8 years ago

@greg0ire no, I didn't remember. I created it PR a year ago, sorry.

This is why we need unit tests…

@Ximousse @ioweb-gr , can you try $menuItem->setExtra('route', $item['route']); and report back?

ioweb-gr commented 8 years ago

@greg0ire it didn't help in my case

Using

Home:
    on_top: true
    icon: '<i class="fa fa-home"></i>'
    items:
        - route: sonata_admin_dashboard
          label: home

and replacing with $menuItem->setExtra('route', $item['route'])

yielded a url of '#' but with the icon presented. My guess is it only presented the group but didn't render the actual menu item in the group

OskarStark commented 7 years ago

I can confirm this bug in my project with the following versions:

sonata-project/admin-bundle              3.18.1  The missing Symfony Admin Generator
sonata-project/block-bundle              3.3.2   Symfony SonataBlockBundle
sonata-project/cache                     1.0.7   Cache library
sonata-project/core-bundle               3.3.0   Symfony SonataCoreBundle
sonata-project/datagrid-bundle           2.2.1   Symfony SonataDatagridBundle
sonata-project/doctrine-extensions       1.0.2   Doctrine2 behavioral extensions
sonata-project/doctrine-orm-admin-bundle 3.1.4   Symfony Sonata / Integrate Doctrine ORM into the SonataAdminBundle
sonata-project/easy-extends-bundle       2.2.0   Symfony SonataEasyExtendsBundle
sonata-project/exporter                  1.7.1   Lightweight Exporter library
sonata-project/google-authenticator      1.1.0   Library to integrate Google Authenticator into a PHP project
sonata-project/user-bundle               3.2.3   Symfony SonataUserBundle
            sonata.admin.group.submit:
                label:              Submit
                label_catalogue:    messages
                icon:               '<i class="fa fa-arrow-right"></i>'
                on_top:             true
                items:
                    - route: admin_skus_submit
                      label: Submit

produces: screenshot 2017-05-19 09 09 01

So, no icon, and wrong target url.

When i use this:

            sonata.admin.group.submit:
                label:              Submit
                label_catalogue:    messages
                icon:               '<i class="fa fa-arrow-right"></i>'
-               on_top:             true
                items:
                    - route: admin_skus_submit
                      label: Submit

it generates an icon for the wrapper and the correct url: screenshot 2017-05-19 09 10 58

OskarStark commented 7 years ago

I can confirm this fixes the icon problem:

sonata-project/admin-bundle/Resources/views/Menu/sonata_menu.html.twig

{% block linkElement %}
    {% spaceless %}
        {% if item.extra('on_top') is defined and not item.extra('on_top') %}
            {% set translation_domain = item.extra('translation_domain', 'messages') %}
-               {% set icon = item.attribute('icon')|default(item.level > 1 ? '<i class="fa fa-angle-double-right" aria-hidden="true"></i>' : '') %}
+               {% set icon = item.extra('icon')|default(item.level > 1 ? '<i class="fa fa-angle-double-right"></i>' : '') %}
        {% else %}

and this fixes the route problem:

sonata-project/admin-bundle/Menu/Provider/GroupMenuProvider.php

- $menuItem->setUri($item['route']);
+ $router = $this->pool->getContainer()->get('router');
+ $menuItem->setUri($router->generate($item['route']));

I will provide a PR right now

EDIT: Added the PRs #4487 & #4488

andrewmy commented 7 years ago

Waiting impatiently for both those PRs to be merged! Ran into both of these issues a few days ago.

OskarStark commented 7 years ago

Both PR's are merged, I will ask for a new release internally

andrewmy commented 7 years ago

Thanks. Watching your step and updating to 3.x-dev for now.