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

[DX] Unify icons #7137

Closed jordisala1991 closed 3 years ago

jordisala1991 commented 3 years ago

Feature Request

Currently we are using Font Awesome to display icons on Sonata. We have placed some of them that can't be changed easily by the user (only overriding templates), and that is just fine.

But we have a few places where the user can modify the icon to be displayed:

  1. Dashboard Actions

The can add a property icon on the actions array just like this:

$actions['list'] = [
    'icon' => 'plus-circle',
];

Note that in this case the user does not add all the code, because we transform that icon property into: fa fa-plus-circle

  1. Menu icon

The user can customize the icon displayed on the menu items:

        dashboard:
            groups:
                foo:
                    icon: '<i class="fas fa-image"></i>'
                    items:
                        - sonata.media.admin.media
                        - sonata.media.admin.gallery

In this case he has to know and add the full html

  1. Default icon for menu

The user can customize the default icon displayed for the menu (by default is a folder)

sonata_admin:
    options:
        default_icon: '<i class="fas fa-folder"></i>'

Similar to the option 2, full html needed

  1. Admin Blocks

Some Blocks we define in the admin have icons AdminSearchBlockService

            ->setDefaults([
                'icon' => '<i class="fas fa-list"></i>',
            ])

Again, using html

  1. Not sure where it is used or if it even works

https://github.com/sonata-project/SonataAdminBundle/blob/3.x/src/Resources/views/Block/block_admin_preview.html.twig#L22-L25 https://github.com/sonata-project/SonataAdminBundle/blob/3.x/src/Resources/views/Block/block_stats.html.twig#L31-L33

In this case seems like the icon will have to contain something like fa-edit and we complete with the missing fa class. I am not even sure if this case works at all, but this is another kind of place where the user can change the icon.

Conclusion

I think we should always add the icons with the same html, the developer doesn't want to know that in some places he needs to put fa-circle, in other just circle and in some places the full: <i class="fas fa-circle"></i>. If we are with Font Awesome, we should not make the user input the full html for the icon like in 4. , 3. or 2.

The easier way for the developer is just input: fas fa-circle

The question is maybe: Why I prefer fas fa-circle over just circle or fa-circle?

Because on Font Awesome 5, there are variant of the icons: fas, fab, far. Not all of them can be used with the free version of Font Awesome 5, but the user can install its own full paid version too, and we shouldn't restrict to fas.

TL;DR:

It would be nice to only have one way to input the icon, adding the full classes but without having to add the html, example: fas fa-circle. This way we make this more intuitive to the developer and easier to learn.

WDYT? @sonata-project/contributors

OskarStark commented 3 years ago

You explanation makes sense 👍

VincentLanglet commented 3 years ago

There is also the listModes https://github.com/sonata-project/SonataAdminBundle/blob/3.x/src/DependencyInjection/Admin/TaggedAdminInterface.php#L73-L74 where a class key is used (and is the only key of this table...).

I agree we should have consistency.

Naturally, I would have prefer the full html <i class="fas fa-image"></i> to let more flexibility. But I'm ok with only the fas fa-image. I would say we just need a check to not add the html if the property is empty.

jordisala1991 commented 3 years ago

In your opinión, this change can be done in a BC way or not? If not, should we do it for 4.0?

VincentLanglet commented 3 years ago

It's seem possible to be BC by doing a check if icon start with 'fa' or if icon start with '<i'. It doesn't seems a big change, so it can be done for 4.0. We still have some PR needed for 4.0. But if nobody has time for this, I won't block the 4.0 release.

vindert commented 3 years ago

I think I can look into this