PrestaShop / hummingbird

77 stars 73 forks source link

Theme icons #595

Open web-cooking-factory opened 5 months ago

web-cooking-factory commented 5 months ago
Questions Answers
Description? Allow developers to adjust their icons in themes and allow module developers to use icons w/o being aware of what icons are being used by theme. Based on POC from @Oksydan (https://github.com/PrestaShop/hummingbird/pull/551)
Type? improvement
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #{https://github.com/PrestaShop/PrestaShop/discussions/34107}
Sponsor company Evolutive
How to test? Try to call an icon from any template {call renderIcon iconName='account_info'}
kpodemski commented 4 months ago

Hello @web-cooking-factory

My understanding is that @Oksydan has some doubts about the current implementation. I'm pinging him, so he could share his experience with us.

Oksydan commented 4 months ago

Hi @kpodemski @web-cooking-factory

First of all. The idea of using smarty function to render icons was all about simplify and improve module developers DX. Right now, a lot of themes are using material icons, fonts awesome or other custom icon fonts. W/o making some weird logic in the module we are not able to support all wide range of icons implementation in the various of themes. I feel like this function should be registered globally and we have to add global_settings.use_icon_map_feature so module developers can check if this feature is being used by theme and will be able to use it. Recently I was trying find a way to check if smarty plugin (function in this case) is registered inside a template file but we are not able to do it :(. So module developers are not able to use it w/o that extra flag in the theme.yml file. Calling not existing function inside the template will throw a SmartyException causing 500 error for the front end of the application.

About the list

Hummingbird theme will be the first theme with this kind of feature. Icons coverage should be higher and we have to change icons names (keys) in the map to more generic. For example:

      "add_comment" => "edit",
      "edit_comments" => "edit",
      "edit_order_step" => "edit"

All of these icons should being used inside template via edit icon key. Icon map keys should not be named based on their presence in a given block. Imagine situation. Module developer need a bell icon. There is no bell icon inside a icon map by default. In this situation he/she have a few options:

That's why I was talking in the POC about a standardized list of icons. I know we won't be able to cover them all, but at least we should try. If one theme can't cover several icons, nothing lost. The icon won't be displayed and IMO it's not the end of the world.

web-cooking-factory commented 4 months ago

Hi @kpodemski and @Oksydan, thanks for your answers.

I agree about registering the function globally and use theme.yml.

But I totally disagree with this: "All of these icons should being used inside template via edit icon key. Icon map keys should not be named based on their presence in a given block." I think quite the opposite. We should have generical entries for general purposes and more specific entries. As a front dev, I can tell we do not necessary use the same icon for edit an address and edit a comment for instance. The idea is to make my life easier by being able to customize the icons without having to edit tpl. If I have only generical entries in the list, the functionnality is useless.

About modules, they should be able to add entries in the list and add custom icons. So the variable allows to get multiple icons set.