Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Theme icons #595

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

web-cooking-factory
Copy link

@web-cooking-factory web-cooking-factory commented Feb 21, 2024

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 (#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'}

@web-cooking-factory web-cooking-factory marked this pull request as ready for review February 21, 2024 16:47
@kpodemski
Copy link
Contributor

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.

Copy link
Contributor

@kpodemski kpodemski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm blocking for @Oksydan comment.

@Oksydan
Copy link
Contributor

Oksydan commented Mar 6, 2024

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:

  • use different icon (meh)
  • use renderIcon function with hope that theme already supports it bell icon
  • use default <span class="material-icons">BELL_CODE</span> and hope that the theme is using material icons

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
Copy link
Author

web-cooking-factory commented Mar 8, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Waiting for author
Development

Successfully merging this pull request may close these issues.

5 participants